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 > > > _______________________________________________ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
