| Message ID | ad3cd046c4dcb9169aaff6c0b739b23d0a06014d.1661149313.git.i.kononenko@yadro.com |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v2,1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown. | expand |
On Mon, 22 Aug 2022 at 06:36, Igor Kononenko <i.kononenko@yadro.com> wrote: > > The previos v1 [PATCH 1/3] have error of getting `struct kcs_bmc_device` > pointer from `struct platform_device *`. Replaced to retriveing pointer by > `platform_get_drvdata()` Can we get a v3 with your original commit message added? You had a good write up in v1 but it was dropped in v2. This change looks like the right thing to do. We should get Andrew to review too, as he spends more time with the KCS controllers. The missed irq issue was seen with the other LPC sub drivers because the clock wasn't enabled. We ended up with this patch: https://lore.kernel.org/all/20201208091748.1920-1-wangzhiqiang.bj@bytedance.com/ Do we need to do something similar for KCS? > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> > --- > drivers/char/ipmi/kcs_bmc_aspeed.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c > index cdc88cde1e9a..8437f3cfe3f4 100644 > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > @@ -636,6 +636,14 @@ static int aspeed_kcs_remove(struct platform_device *pdev) > return 0; > } > > +static void aspeed_kcs_shutdown(struct platform_device *pdev) > +{ > + struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev); > + struct kcs_bmc_device *kcs_bmc = &priv->kcs_bmc; > + > + aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); > +} > + > static const struct of_device_id ast_kcs_bmc_match[] = { > { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, > { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, > @@ -651,6 +659,7 @@ static struct platform_driver ast_kcs_bmc_driver = { > }, > .probe = aspeed_kcs_probe, > .remove = aspeed_kcs_remove, > + .shutdown = aspeed_kcs_shutdown, > }; > module_platform_driver(ast_kcs_bmc_driver); > > -- > 2.25.1 >
On 23.08.2022 03:22, Joel Stanley wrote: > «Внимание! Данное письмо от внешнего адресата!» > > On Mon, 22 Aug 2022 at 06:36, Igor Kononenko <i.kononenko@yadro.com> wrote: >> >> The previos v1 [PATCH 1/3] have error of getting `struct kcs_bmc_device` >> pointer from `struct platform_device *`. Replaced to retriveing pointer by >> `platform_get_drvdata()` > > Can we get a v3 with your original commit message added? You had a > good write up in v1 but it was dropped in v2. > Thanks for the review. Ok, I'll include the origin commit message to a v3. > This change looks like the right thing to do. We should get Andrew to > review too, as he spends more time with the KCS controllers. > > The missed irq issue was seen with the other LPC sub drivers because > the clock wasn't enabled. We ended up with this patch: > > https://lore.kernel.org/all/20201208091748.1920-1-wangzhiqiang.bj@bytedance.com/ >> Do we need to do something similar for KCS? As far as I see by the LPC 'nobody cared irq' issue there had the feature of enabling LCLK individually earlier, there is patch about: https://lore.kernel.org/all/20211101233751.49222-5-jae.hyun.yoo@intel.com/ Originally I found the bug on the linux-dev.v5.4 that includes the 'LCLK individually enabling' feature. It seems to me the issue is that lpc-snoop and the lpc-kcs has same IRQ#35 that is used in separated drivers(by the IRQF_SHARED flag). The IRQ handler determinate request purpose(for kcs or snoop) by LPC interrupt registers state, and if such interrupt is not for any one of them, the irq-handler passthrough request to a next handler by returning `IRQ_NONE`. So, even if lpc-kcs will be having adjusted own individual LCLK, that is doesn't solve issue, because when lpc-snoop will had configured irq-handler the irq-manager will know that for IRQ#35 already registered a good handler, but such handler will skip all requests by `IRQ_NONE` because such irqs are intended for lpc-kcs. I guess that is the point of bug. > >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> >> --- >> drivers/char/ipmi/kcs_bmc_aspeed.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c >> index cdc88cde1e9a..8437f3cfe3f4 100644 >> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c >> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c >> @@ -636,6 +636,14 @@ static int aspeed_kcs_remove(struct platform_device *pdev) >> return 0; >> } >> >> +static void aspeed_kcs_shutdown(struct platform_device *pdev) >> +{ >> + struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev); >> + struct kcs_bmc_device *kcs_bmc = &priv->kcs_bmc; >> + >> + aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); >> +} >> + >> static const struct of_device_id ast_kcs_bmc_match[] = { >> { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, >> { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, >> @@ -651,6 +659,7 @@ static struct platform_driver ast_kcs_bmc_driver = { >> }, >> .probe = aspeed_kcs_probe, >> .remove = aspeed_kcs_remove, >> + .shutdown = aspeed_kcs_shutdown, >> }; >> module_platform_driver(ast_kcs_bmc_driver); >> >> -- >> 2.25.1 >>
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c index cdc88cde1e9a..8437f3cfe3f4 100644 --- a/drivers/char/ipmi/kcs_bmc_aspeed.c +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c @@ -636,6 +636,14 @@ static int aspeed_kcs_remove(struct platform_device *pdev) return 0; } +static void aspeed_kcs_shutdown(struct platform_device *pdev) +{ + struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev); + struct kcs_bmc_device *kcs_bmc = &priv->kcs_bmc; + + aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF), 0); +} + static const struct of_device_id ast_kcs_bmc_match[] = { { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, @@ -651,6 +659,7 @@ static struct platform_driver ast_kcs_bmc_driver = { }, .probe = aspeed_kcs_probe, .remove = aspeed_kcs_remove, + .shutdown = aspeed_kcs_shutdown, }; module_platform_driver(ast_kcs_bmc_driver);
The previos v1 [PATCH 1/3] have error of getting `struct kcs_bmc_device` pointer from `struct platform_device *`. Replaced to retriveing pointer by `platform_get_drvdata()` Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com> --- drivers/char/ipmi/kcs_bmc_aspeed.c | 9 +++++++++ 1 file changed, 9 insertions(+)