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
--
மணிவண்ணன் சதாசிவம்
