Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Andy Shevchenko <andy.shevchenko@gmail.com>]

+Cc Kees (see below about allocation size checks)

On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
> On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > <alexandru.ardelean@analog.com> wrote:

...

> > > Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> >
>
> i can send a v12 for this in a few days;
>
> > Is it tag or simple link? I would suggest not to use Link: if it's not a tag.
>
> simple link
> any suggestions/alternatives?
> i wasn't aware of conventions about this;

Use like [1] ...
...

[1]: https://...

Or maybe introduce is as a tag DocLink:, for example?
Or Datasheet: ?

...

> > > +static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv
> > > *conv)
> > > +{
> > > +       if (!conv)
> > > +               return NULL;
> >
> > This is so unusual. Why do you need it?
>
> see [1]
>
> >
> > > +       return container_of(conv, struct adi_axi_adc_client, conv);
> > > +}
> > > +
> > > +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> > > +{
> > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > +
> > > +       if (!cl)
> > > +               return NULL;
> >
> > So about this.
>
> [1]
> because 'adi_axi_adc_conv_priv()' (and implicitly conv_to_client()) gets called
> from other drivers; we can't expect to be sure that conv & cl aren't NULL;

In both cases it's pointer arithmetic, right? Even look at the example
of netdev you gave below, they haven't done these (redundant) checks.
The outcome that crashes if any will be more distinct.

> > > +       return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> > > IIO_ALIGN);
> >
> > This all looks a bit confusing. Is it invention of offsetof() ?
>
> umm; tbh, it's more of a copy/clone of iio_priv()
>
> it's not un-common though;
> see [and this one has more exposure]:
> --------------------------------------------------------
> static inline void *netdev_priv(const struct net_device *dev)
> {
>         return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> }
> --------------------------------------------------------

Good point.

> > > +}

...

> > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > *dev,
> > > +                                                         int sizeof_priv)
> > > +{
> > > +       struct adi_axi_adc_client *cl;
> > > +       size_t alloc_size;
> > > +
> > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > +       if (sizeof_priv) {
> > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > +               alloc_size += sizeof_priv;
> > > +       }
> > > +       alloc_size += IIO_ALIGN - 1;
> >
> > Have you looked at linux/overflow.h?
>
> i did now;
> any hints where i should look closer?

It seems it lacks of this kind of allocation size checks... Perhaps add one?
Kees, what do you think?

> > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > +       if (!cl)
> > > +               return ERR_PTR(-ENOMEM);

...

> > > +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> > > +{
> > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > +
> > > +       if (!cl)
> > > +               return;
> >
> > When is this possible?
>
> good point; it isn't;
> it's a left-over from when adi_axi_adc_conv_unregister() was exported
> still, i wouldn't mind leaving it [for paranoia], if there isn't a strong
> opinion to remove it;

I think it makes code dirty (too much protective programming). We have
a lot places where we can shoot our feet, but at least not hiding the
issue is a benefit in my opinion.

...



> > > +static struct attribute *adi_axi_adc_attributes[] = {
> > > +       ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> > > +       NULL,
> >
> > Terminators good w/o comma.
>
> i don't feel strongly pro/against
> sure

There is a rationale behind this. If there is a weird case of adding
entry behind the terminator, you will see it immediately at compile
time (consider automatic rebase).

> > > +};
> >
> > ...
> >
> > > +/* Match table for of_platform binding */
> > > +static const struct of_device_id adi_axi_adc_of_match[] = {
> > > +       { .compatible = "adi,axi-adc-10.0.a", .data =
> > > &adi_axi_adc_10_0_a_info },
> > > +       { /* end of list */ },
> >
> > Ditto.

Ditto.

> > > +};

...

> > > +       if (!dev->of_node) {
> > > +               dev_err(dev, "DT node is null\n");
> > > +               return ERR_PTR(-ENODEV);
> > > +       }

I guess this check is redundant since following OF calls will fail anyway.

> > > +
> > > +       id = of_match_node(adi_axi_adc_of_match, dev->of_node);
> >
> > You may use this from struct driver and move the table after this function.
>
>
> right; it didn't occur to me, since i was already using
> of_device_get_match_data() in ad9467

Even better. But see above note.

> > > +       if (!id)
> > > +               return ERR_PTR(-ENODEV);

-- 
With Best Regards,
Andy Shevchenko