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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
>
>
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
>
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
>
> >
>
>
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
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
>
>
>
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