* [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement
@ 2020-04-08 11:59 Tang Bin
2020-04-13 11:32 ` Corey Minyard
0 siblings, 1 reply; 7+ messages in thread
From: Tang Bin @ 2020-04-08 11:59 UTC (permalink / raw)
To: minyard, arnd, gregkh; +Cc: openipmi-developer, linux-kernel, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement
2020-04-08 11:59 [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement Tang Bin
@ 2020-04-13 11:32 ` Corey Minyard
2020-04-13 11:56 ` Tang Bin
0 siblings, 1 reply; 7+ messages in thread
From: Corey Minyard @ 2020-04-13 11:32 UTC (permalink / raw)
To: Tang Bin; +Cc: arnd, gregkh, openipmi-developer, linux-kernel
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
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement
2020-04-13 11:32 ` Corey Minyard
@ 2020-04-13 11:56 ` Tang Bin
2020-04-13 14:23 ` Corey Minyard
0 siblings, 1 reply; 7+ messages in thread
From: Tang Bin @ 2020-04-13 11:56 UTC (permalink / raw)
To: minyard; +Cc: arnd, gregkh, openipmi-developer, linux-kernel
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement
2020-04-13 11:56 ` Tang Bin
@ 2020-04-13 14:23 ` Corey Minyard
2020-04-13 15:44 ` Tang Bin
0 siblings, 1 reply; 7+ messages in thread
From: Corey Minyard @ 2020-04-13 14:23 UTC (permalink / raw)
To: Tang Bin; +Cc: arnd, gregkh, openipmi-developer, linux-kernel
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
>
> >
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement
2020-04-13 14:23 ` Corey Minyard
@ 2020-04-13 15:44 ` Tang Bin
2020-04-13 21:59 ` Corey Minyard
0 siblings, 1 reply; 7+ messages in thread
From: Tang Bin @ 2020-04-13 15:44 UTC (permalink / raw)
To: minyard; +Cc: arnd, gregkh, openipmi-developer, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement
2020-04-13 15:44 ` Tang Bin
@ 2020-04-13 21:59 ` Corey Minyard
2020-04-14 9:42 ` Tang Bin
0 siblings, 1 reply; 7+ messages in thread
From: Corey Minyard @ 2020-04-13 21:59 UTC (permalink / raw)
To: Tang Bin; +Cc: arnd, gregkh, openipmi-developer, linux-kernel
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
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement
2020-04-13 21:59 ` Corey Minyard
@ 2020-04-14 9:42 ` Tang Bin
0 siblings, 0 replies; 7+ messages in thread
From: Tang Bin @ 2020-04-14 9:42 UTC (permalink / raw)
To: minyard; +Cc: arnd, gregkh, openipmi-developer, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-14 9:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08 11:59 [PATCH v3]ipmi:bt-bmc:Avoid unnecessary judgement Tang Bin
2020-04-13 11:32 ` Corey Minyard
2020-04-13 11:56 ` Tang Bin
2020-04-13 14:23 ` Corey Minyard
2020-04-13 15:44 ` Tang Bin
2020-04-13 21:59 ` Corey Minyard
2020-04-14 9:42 ` Tang Bin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).