Re: [PATCH v5 16/17] qcom: llcc/edac: Support polling mode for ECC handling

[Manivannan Sadhasivam]

On Mon, Jan 16, 2023 at 11:41:26AM +0100, Borislav Petkov wrote:
> On Sun, Jan 15, 2023 at 09:38:25AM +0530, Manivannan Sadhasivam wrote:
> > > You need to request the IRQ first and then set edac_op_state above. I.e., this
> > > devm_request_irq() needs to move in the if (ecc_irq > 0) branch above.
> > 
> > May I know why? I also checked other drivers, most of them are doing the same.
> 
> If the others do it, that doesn't mean it is clean.
> 
> What happens to edac_op_state if devm_request_irq() fails?
> 
> I know I know, the probe function will fail and the driver won't load but still,
> this is sloppy. And it could come down to bite us later, when someone
> reorganizes that function.
> 

OK. I just wanted to know the reasoning behind it.

Thanks,
Mani

> So, do all the error checking method determination - polling or interrupt - in
> one place.  Something like this (totally untested ofc, pasting here the whole
> thing to show what I mean):
> 
> static int qcom_llcc_edac_probe(struct platform_device *pdev)
> {
>         struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
>         struct edac_device_ctl_info *edev_ctl;
>         struct device *dev = &pdev->dev;
>         int ecc_irq;
>         int rc;
> 
>         rc = qcom_llcc_core_setup(llcc_driv_data->bcast_regmap);
>         if (rc)
>                 return rc;
> 
>         /* Allocate edac control info */
>         edev_ctl = edac_device_alloc_ctl_info(0, "qcom-llcc", 1, "bank",
>                                               llcc_driv_data->num_banks, 1,
>                                               NULL, 0,
>                                               edac_device_alloc_index());
> 
>         if (!edev_ctl)
>                 return -ENOMEM;
> 
>         edev_ctl->dev = dev;
>         edev_ctl->mod_name = dev_name(dev);
>         edev_ctl->dev_name = dev_name(dev);
>         edev_ctl->ctl_name = "llcc";
>         edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
> 
>         /* Check if LLCC driver has passed ECC IRQ */
>         ecc_irq = llcc_driv_data->ecc_irq;
>         if (ecc_irq > 0) {
>                 rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
>                                       IRQF_TRIGGER_HIGH, "llcc_ecc", edev_ctl);
>                 if (!rc) {
>                         edac_op_state = EDAC_OPSTATE_INT;
>                         goto irq_done;
>                 }
>         }
> 
>         /* Fall back to polling mode otherwise */
>         edev_ctl->poll_msec = ECC_POLL_MSEC;
>         edev_ctl->edac_check = llcc_ecc_check;
>         edac_op_state = EDAC_OPSTATE_POLL;
> 
> irq_done:
>         rc = edac_device_add_device(edev_ctl);
>         if (rc) {
>                 edac_device_free_ctl_info(edev_ctl);
>                 return rc;
>         }
> 
>         platform_set_drvdata(pdev, edev_ctl);
> 
>         return rc;
> }
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

-- 
மணிவண்ணன் சதாசிவம்