Re: [PATCH v17 3/3] EDAC/npcm: Add NPCM memory controller driver

[Kun-Fa Lin]

Hi Boris,

Thanks for the review.

> > +     u64 addr = 0;
> > +     u64 data = 0;
> > +     u32 val_h = 0;
> > +     u32 val_l, id, synd;
>
>         u32 val_h = 0, val_l, id, synd;
>         u64 addr = 0, data = 0;
>
> Also, for all your functions:
>
> The EDAC tree preferred ordering of variable declarations at the
> beginning of a function is reverse fir tree order::
>
>         struct long_struct_name *descriptive_name;
>         unsigned long foo, bar;
>         unsigned int tmp;
>         int ret;
>
> The above is faster to parse than the reverse ordering::
>
>         int ret;
>         unsigned int tmp;
>         unsigned long foo, bar;
>         struct long_struct_name *descriptive_name;
>
> And even more so than random ordering::
>
>         unsigned long foo, bar;
>         int ret;
>         struct long_struct_name *descriptive_name;
>         unsigned int tmp;

I'll check all functions and follow this order.

> > +     edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1, addr >> PAGE_SHIFT,
> > +                          addr & ~PAGE_MASK, synd, 0, 0, -1, priv->message,
>
> No need for that linebreak with the trailing piece.
>
> > +                          "");

> > +     u64 addr = 0;
> > +     u64 data = 0;
> > +     u32 val_h = 0;
> > +     u32 val_l, id, synd;
>
> As above.

Check.

> > +     regmap_read(npcm_regmap, pdata->ctl_int_status, &status);
> > +     if (status & pdata->int_status_ce_mask) {
> > +             handle_ce(mci);
> > +
> > +             /* acknowledge the CE interrupt */
> > +             regmap_write(npcm_regmap, pdata->ctl_int_ack,
> > +                          pdata->int_ack_ce_mask);
> > +             return IRQ_HANDLED;
> > +     } else if (status & pdata->int_status_ue_mask) {
> > +             handle_ue(mci);
> > +
> > +             /* acknowledge the UE interrupt */
> > +             regmap_write(npcm_regmap, pdata->ctl_int_ack,
> > +                          pdata->int_ack_ue_mask);
> > +             return IRQ_HANDLED;
> > +     }
>
>         WARN_ON_ONCE(1);
>
> to catch weird cases.

OK.

> > +     /* write syndrome to XOR_CHECK_BITS */
> > +     if (priv->error_type == 0) {
>
>         if (priv->error_type == ERROR_TYPE_CORRECTABLE
>
> Use defines. Below too.
>
> > +             if (priv->location == 0 && priv->bit > 63) {

Will add defines.

> > +                     edac_printk(KERN_INFO, EDAC_MOD_NAME,
> > +                                 "data bit should not exceed 63\n");
>
>                                 "data bit should not exceed 63 (%d)\n", priv->bit...)
>
> Below too.
>
> > +                     return count;
> > +             }
> > +
> > +             if (priv->location == 1 && priv->bit > 7) {
> > +                     edac_printk(KERN_INFO, EDAC_MOD_NAME,
> > +                                 "checkcode bit should not exceed 7\n");

OK.

> > +             syndrome = priv->location ? 1 << priv->bit :
> > +                        data_synd[priv->bit];
>
>                 syndrome = priv->location ? 1 << priv->bit
>                                           : data_synd[priv->bit];

Just to confirm the indentation, is it right as follows?

syndrome = priv->location ? 1 << priv->bit
                                           : data_synd[priv->bit];

And I was wondering if I should just remove the line break and let it stick out?

> I'd find it helpful if there were a short recipe in a comment here
> explaining how the injection should be done...
>
> > +static void setup_debugfs(struct mem_ctl_info *mci)
> > +{

OK, will add some injection examples here.

> > +     regmap_update_bits(npcm_regmap, pdata->ctl_ecc_en, pdata->ecc_en_mask,
> > +                        0);
>
> You have a bunch of those things: the 80 cols rule is not a rigid and a
> static one - you should rather apply common sense. As in:
>
> Does it make sense to have this ugly linebreak with only the 0 argument there?
>
>         regmap_update_bits(npcm_regmap, pdata->ctl_ecc_en, pdata->ecc_en_mask,
>                            0);
>
> or should I simply let it stick out:
>
>         regmap_update_bits(npcm_regmap, pdata->ctl_ecc_en, pdata->ecc_en_mask, 0);
>
> and have much more readable code?
>
> Please apply common sense to all your linebreaks above.

Thanks for the guide.

> > +     edac_mc_del_mc(&pdev->dev);
> > +     edac_mc_free(mci);
>
> <--- What happens if someone tries to inject errors right at this
> moment, when you've freed the mci?
>
> Hint: you should destroy resources in the opposite order you've
> allocated them.

Understand. I'll correct the destroy order.

Regards,
Marvin