Re: [PATCH v9 2/2] EDAC: al-mc-edac: Introduce Amazon's Annapurna Labs Memory Controller EDAC

[Shenhar, Talel]

On 8/15/2020 9:33 PM, Borislav Petkov wrote:
> On Tue, Jul 28, 2020 at 12:51:55PM +0300, Talel Shenhar wrote:
> > +static void al_mc_edac_check(struct mem_ctl_info *mci)
> > +{
> > +     struct al_mc_edac *al_mc = mci->pvt_info;
> > +
> > +     if (al_mc->irq_ue <= 0)
> > +             handle_ue(mci);
> > +
> > +     if (al_mc->irq_ce <= 0)
> > +             handle_ce(mci);
> > +}
> > +
> > +static irqreturn_t al_mc_edac_irq_handler_ue(int irq, void *info)
> > +{
> > +     struct platform_device *pdev = info;
> > +     struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> > +
> > +     if (handle_ue(mci))
> > +             return IRQ_HANDLED;
> > +     return IRQ_NONE;
> > +}
> > +
> > +static irqreturn_t al_mc_edac_irq_handler_ce(int irq, void *info)
> > +{
> > +     struct platform_device *pdev = info;
> > +     struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> > +
> > +     if (handle_ce(mci))
> > +             return IRQ_HANDLED;
> > +     return IRQ_NONE;
> > +}
> > +
> > +static enum scrub_type al_mc_edac_get_scrub_mode(void __iomem *mmio_base)
> > +{
> > +     u32 ecccfg0;
> > +
> > +     ecccfg0 = readl(mmio_base + AL_MC_ECC_CFG);
> > +
> > +     if (FIELD_GET(AL_MC_ECC_CFG_SCRUB_DISABLED, ecccfg0))
> > +             return SCRUB_NONE;
> > +     else
> > +             return SCRUB_HW_SRC;
> > +}
> > +
> > +static void devm_al_mc_edac_free(void *data)
> > +{
> > +     edac_mc_free(data);
> > +}
> > +
> > +static void devm_al_mc_edac_del(void *data)
> > +{
> > +     edac_mc_del_mc(data);
> > +}
>  From a previous review:
>
> I said:
>
> > Drop the "al_mc_edac_" prefix from most of the static functions. You can
> > leave it in the probe function or the IRQ handler so that it is visible
> > in stack traces but all those small functions don't need that prefix.
> You replied with:
>
> > Shall be part of v7.
> and yet it ain't part of any v<num>.
>
> Why?

Thanks for taking a look.

From cover letter:

- removed static function names prefix from internal functions (external
  used function, such as devm/interrupts-handlers/probe, left with the
  prefix to allow stack trace visibility)

As you can see, part of the functions got their prefix removed, e.g. 
prepare_msg, handle_ce, handle_ue.

I did take your advise for leaving prefix for having visibility for 
functions being used outside. hence, some were left with the prefix.

Let me know what you think.

> --
> Regards/Gruss,
>      Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette