[PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[Tang Bin]

bt_bmc_probe() is only called with an openfirmware platform device.
Therefore there is no need to check that the passed in device is NULL or
that it has an openfirmware node.

Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
---
Changes from v2:
 - improve the commit message.

Changes from v1:
 - improve the commit message.
---
 drivers/char/ipmi/bt-bmc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
index d36aeacb2..890ad55aa 100644
--- a/drivers/char/ipmi/bt-bmc.c
+++ b/drivers/char/ipmi/bt-bmc.c
@@ -430,9 +430,6 @@ static int bt_bmc_probe(struct platform_device *pdev)
 	struct device *dev;
 	int rc;
 
-	if (!pdev || !pdev->dev.of_node)
-		return -ENODEV;
-
 	dev = &pdev->dev;
 	dev_info(dev, "Found bt bmc device\n");
 
-- 
2.20.1.windows.1

Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[Corey Minyard]

On Wed, Apr 08, 2020 at 07:59:58PM +0800, Tang Bin wrote:
> bt_bmc_probe() is only called with an openfirmware platform device.
> Therefore there is no need to check that the passed in device is NULL or
> that it has an openfirmware node.

I waited until after the merge window closed, this is queued for 5.8.  I
changed the title to be "Avoid unnecessary check".  "Judgement",
although technically correct, has a legal or moral connotation.

Thanks,

-corey

> 
> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> ---
> Changes from v2:
>  - improve the commit message.
> 
> Changes from v1:
>  - improve the commit message.
> ---
>  drivers/char/ipmi/bt-bmc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> index d36aeacb2..890ad55aa 100644
> --- a/drivers/char/ipmi/bt-bmc.c
> +++ b/drivers/char/ipmi/bt-bmc.c
> @@ -430,9 +430,6 @@ static int bt_bmc_probe(struct platform_device *pdev)
>  	struct device *dev;
>  	int rc;
>  
> -	if (!pdev || !pdev->dev.of_node)
> -		return -ENODEV;
> -
>  	dev = &pdev->dev;
>  	dev_info(dev, "Found bt bmc device\n");
>  
> -- 
> 2.20.1.windows.1
> 
> 
> 

Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[Tang Bin]

Hi, Corey:

On 2020/4/13 19:32, Corey Minyard wrote:
> On Wed, Apr 08, 2020 at 07:59:58PM +0800, Tang Bin wrote:
>> bt_bmc_probe() is only called with an openfirmware platform device.
>> Therefore there is no need to check that the passed in device is NULL or
>> that it has an openfirmware node.
> I waited until after the merge window closed, this is queued for 5.8.
Can I consider that the patch will be applied in 5.8？
>   I
> changed the title to be "Avoid unnecessary check".
You have modified it, which means I don't need to submit a new patch？
>   "Judgement",
> although technically correct, has a legal or moral connotation.

I'm sorry, I won't use that word again.


Thanks for your instruction.

Tang Bin

>

Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[Corey Minyard]

On Mon, Apr 13, 2020 at 07:56:44PM +0800, Tang Bin wrote:
> Hi, Corey:
> 
> On 2020/4/13 19:32, Corey Minyard wrote:
> > On Wed, Apr 08, 2020 at 07:59:58PM +0800, Tang Bin wrote:
> > > bt_bmc_probe() is only called with an openfirmware platform device.
> > > Therefore there is no need to check that the passed in device is NULL or
> > > that it has an openfirmware node.
> > I waited until after the merge window closed, this is queued for 5.8.
> Can I consider that the patch will be applied in 5.8？

It's in my queue, so that's the plan.

> >   I
> > changed the title to be "Avoid unnecessary check".
> You have modified it, which means I don't need to submit a new patch？

Correct.

> >   "Judgement",
> > although technically correct, has a legal or moral connotation.
> 
> I'm sorry, I won't use that word again.

It's not a problem.  English is a language with a lot of things like
this.

-corey

> 
> 
> Thanks for your instruction.
> 
> Tang Bin
> 
> > 
> 
> 

Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[Tang Bin]

Hi Corey:

On 2020/4/13 22:23, Corey Minyard wrote:
>> Can I consider that the patch will be applied in 5.8？
> It's in my queue, so that's the plan.
>
>>>    I
>>> changed the title to be "Avoid unnecessary check".
>> You have modified it, which means I don't need to submit a new patch？
> Correct.

Thank you very much, I am waiting for the applied.


Then, I have some questions to ask you:

     I have checked the file bt-bmc.c carefully, and found that there 
are another two problems.Please help me analyze them, if you think it is 
feasible, then I will submit the patch.

     Q1: About Format Problem

            In the 469~471 line, the first letter should be indented, 
please check if the writing here is reasonable?


     Q2: About the function bt_bmc_config_irq()

           1）In the function bt_bmc_probe(), the return value of 
bt_bmc_config_irq() made no judgement, whether it is suitable? （If your 
view is don't need to judge, the following will change.）


           2）According to the kernel interface of platform_get_irq(),the 
return value is negative,

                    if (!bt_bmc->irq)
                         return -ENODEV;

                so the check here is invalid.The standard way to write is:

                      if (bt_bmc->irq < 0)
                           return bt_bmc->irq;

                But consider if failed, "bt_bmc->irq" must be assigned 
to "0"，the easiest way is to delete the        403~404 line, handled 
directly by the function devm_request_irq().


         Q3：About dev_warm()

                 KERN_WARNING is higher than KERN_INFO, the same to 
dev_warn() and dev_info(). When the function bt_bmc_probe() uses 
dev_info() to print error message, the dev_warm() in the line of 409 
should be redundant.


I am waiting for your replay, and thank you for your guidance.

Tang Bin

Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[Corey Minyard]

On Mon, Apr 13, 2020 at 11:44:49PM +0800, Tang Bin wrote:
> Hi Corey:
> 
> On 2020/4/13 22:23, Corey Minyard wrote:
> > > Can I consider that the patch will be applied in 5.8？
> > It's in my queue, so that's the plan.
> > 
> > > >    I
> > > > changed the title to be "Avoid unnecessary check".
> > > You have modified it, which means I don't need to submit a new patch？
> > Correct.
> 
> Thank you very much, I am waiting for the applied.
> 
> 
> Then, I have some questions to ask you:
> 
>     I have checked the file bt-bmc.c carefully, and found that there are
> another two problems.Please help me analyze them, if you think it is
> feasible, then I will submit the patch.
> 
>     Q1: About Format Problem
> 
>            In the 469~471 line, the first letter should be indented, please
> check if the writing here is reasonable?
> 

I'm not sure how that happened.  It was that way from the original
submitter and nobody noticed in review.  It's obviously wrong.

> 
>     Q2: About the function bt_bmc_config_irq()
> 
>           1）In the function bt_bmc_probe(), the return value of
> bt_bmc_config_irq() made no judgement, whether it is suitable? （If your
> view is don't need to judge, the following will change.）
> 

Hmm, that's probably not a big deal.  If it fails irqs are just not
used.  It probably shouldn't return a value, though.

> 
>           2）According to the kernel interface of platform_get_irq(),the
> return value is negative,
> 
>                    if (!bt_bmc->irq)
>                         return -ENODEV;
> 
>                so the check here is invalid.The standard way to write is:
> 
>                      if (bt_bmc->irq < 0)
>                           return bt_bmc->irq;
> 
>                But consider if failed, "bt_bmc->irq" must be assigned to
> "0"，the easiest way is to delete the        403~404 line, handled directly
> by the function devm_request_irq().

The problem you point out is real, the check should be < 0.

You don't want it handled by devm_request_irq, that could result in logs
that are invalid.

Also, it should use platform_get_irq_optional() to avoid a spurrious log
when there is no irq.

> 
> 
>         Q3：About dev_warm()
> 
>                 KERN_WARNING is higher than KERN_INFO, the same to
> dev_warn() and dev_info(). When the function bt_bmc_probe() uses dev_info()
> to print error message, the dev_warm() in the line of 409 should be
> redundant.

That is all correct as it is.  If there is an irq specified and it can't
be requested, that is a problem.  If there is no irq specified, that is
fine, just info is good.

Thanks,

-corey

> 
> 
> I am waiting for your replay, and thank you for your guidance.
> 
> Tang Bin
> 
> 
> 

Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement

[Tang Bin]

Hi Corey:

On 2020/4/14 5:59, Corey Minyard wrote:
> That is all correct as it is.  If there is an irq specified and it can't
> be requested, that is a problem.  If there is no irq specified, that is
> fine, just info is good.

Okay, I know what you mean, and I will submit the corresponding patch 
tonight according to the questions I raised.

Thanks,

Tang Bin