[PATCH v2 1/3] ipmi:kcs_bmc: Add cleanup regmap(interrupt-regs) on a shutdown.

[i.kononenko]

On 23.08.2022 03:22, Joel Stanley wrote:
> «Внимание! Данное письмо от внешнего адресата!»
> 
> On Mon, 22 Aug 2022 at 06:36, Igor Kononenko <i.kononenko at 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 at intel.com>
>> Signed-off-by: Igor Kononenko <i.kononenko at 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
>>

-- 
Best regards,

Igor Kononenko