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

[Ardelean, Alexandru]

On Sun, 2020-03-22 at 12:45 +0200, Andy Shevchenko wrote:
> +Cc Kees (see below about allocation size checks)
> 
> On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> <alexandru.Ardelean@xxxxxxxxxx> 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@xxxxxxxxxx> 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?

i borrowed this allocation logic from IIO core [iio_device_alloc()];

i may be stupid, but i still don't understand how to use overflow.h or what to
get from it;
the checks in there seem to be a bit too much for what's needed here;
or maybe there is something else in some newer linux-tree?
or maybe an example of how it's used?

> 
> > > > +       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);