diff mbox series

bus: fix code unused

Message ID 20220512121958.2491-1-taoyunxiang@cmss.chinamobile.com (mailing list archive)
State Rejected, archived
Delegated to: David Marchand
Headers
Series bus: fix code unused |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS

Commit Message

taoyunxiang@cmss.chinamobile.com May 12, 2022, 12:19 p.m. UTC 
  The return value of pci_plug in pci_common.c
will always be int vaule, can not be NULL.
We could use not 0 to check it.

Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>

---
 lib/eal/common/eal_common_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

David Marchand May 16, 2022, 7:55 a.m. UTC | #1 
On Mon, May 16, 2022 at 9:38 AM taoyunxiang
<taoyunxiang@cmss.chinamobile.com> wrote:
>
> The return value of pci_plug in pci_common.c
> will always be int vaule, can not be NULL.
> We could use not 0 to check it.

I don't see the relation between patch and commitlog.
Please clarify what is the issue you want to fix.


>
> Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
> Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>
>
> ---
>  lib/eal/common/eal_common_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
> index 148a23830a..99677bae58 100644
> --- a/lib/eal/common/eal_common_dev.c
> +++ b/lib/eal/common/eal_common_dev.c
> @@ -143,7 +143,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
>         if (ret)
>                 goto err_devarg;
>
> -       if (da->bus->plug == NULL) {
> +       if (da->bus->plug != 0) {

The current (before patch) check is correct: it is allowed that a bus
does not support hotplug.
Inverting this check as you propose breaks hotplug.


>                 RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
>                         da->bus->name);
>                 ret = -ENOTSUP;
taoyunxiang@cmss.chinamobile.com May 17, 2022, 1:54 a.m. UTC | #2 
Hi David ,
      Thanks for your review, see the bellows.


--------------



taoyunxiang@cmss.chinamobile.com



>On Mon, May 16, 2022 at 9:38 AM taoyunxiang



><taoyunxiang@cmss.chinamobile.com> wrote:



>>



>> The return value of pci_plug in pci_common.c



>> will always be int vaule, can not be NULL.



>> We could use not 0 to check it.



>



>I don't see the relation between patch and commitlog.



>Please clarify what is the issue you want to fix.

I think we have some code unused, so fix it.


>



>



>>



>> Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>



>> Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com>



>>



>> ---



>>  lib/eal/common/eal_common_dev.c | 2 +-



>>  1 file changed, 1 insertion(+), 1 deletion(-)



>>



>> diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c



>> index 148a23830a..99677bae58 100644



>> --- a/lib/eal/common/eal_common_dev.c



>> +++ b/lib/eal/common/eal_common_dev.c



>> @@ -143,7 +143,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)



>>         if (ret)



>>                 goto err_devarg;



>>



>> -       if (da->bus->plug == NULL) {



>> +       if (da->bus->plug != 0) {



>



>The current (before patch) check is correct: it is allowed that a bus



>does not support hotplug.



>Inverting this check as you propose breaks hotplug.


"da->bus->plug" will call pci_plug and  pci_probe_all_drivers in pci_common.c , is it right ?

The pci_probe_all_drivers will never return NULL,  so the check and related code will not go throuth,
no matter the plug is ok or not.


>



>



>>                 RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",



>>                         da->bus->name);



>>                 ret = -ENOTSUP;



>



>



>-- 



>David Marchand



>
David Marchand May 18, 2022, 12:07 p.m. UTC | #3 
On Tue, May 17, 2022 at 3:54 AM taoyunxiang@cmss.chinamobile.com
<taoyunxiang@cmss.chinamobile.com> wrote:
> >> @@ -143,7 +143,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
> >>         if (ret)
> >>                 goto err_devarg;
> >>
> >> -       if (da->bus->plug == NULL) {
> >> +       if (da->bus->plug != 0) {
>
>
>
> >The current (before patch) check is correct: it is allowed that a bus
> >does not support hotplug.
> >Inverting this check as you propose breaks hotplug.
>
> "da->bus->plug" will call pci_plug and  pci_probe_all_drivers in pci_common.c , is it right ?

Checking for da->bus->plug value is different from calling da->bus->plug(dev).

>
> The pci_probe_all_drivers will never return NULL,  so the check and related code will not go throuth,
> no matter the plug is ok or not.

This check is needed, and is correct in its current form.


Thanks.
diff mbox series

Patch

diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
index 148a23830a..99677bae58 100644
--- a/lib/eal/common/eal_common_dev.c
+++ b/lib/eal/common/eal_common_dev.c
@@ -143,7 +143,7 @@  local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	if (ret)
 		goto err_devarg;
 
-	if (da->bus->plug == NULL) {
+	if (da->bus->plug != 0) {
 		RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
 			da->bus->name);
 		ret = -ENOTSUP;