linux-kernel - Re: [PATCH 2/4] iio: frequency: adf4377: add support for ADF4377

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20221115160937.00000313@Huawei.com>
Date:   Tue, 15 Nov 2022 16:09:37 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Nuno Sá <noname.nuno@...il.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Antoniu Miclaus <antoniu.miclaus@...log.com>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] iio: frequency: adf4377: add support for ADF4377


> > > > +static int adf4377_probe(struct spi_device *spi)
> > > > +{
> > > > +       struct iio_dev *indio_dev;
> > > > +       struct regmap *regmap;
> > > > +       struct adf4377_state *st;
> > > > +       int ret;
> > > > +
> > > > +       indio_dev = devm_iio_device_alloc(&spi->dev,
> > > > sizeof(*st));
> > > > +       if (!indio_dev)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       regmap = devm_regmap_init_spi(spi,
> > > > &adf4377_regmap_config);
> > > > +       if (IS_ERR(regmap))
> > > > +               return PTR_ERR(regmap);
> > > > +
> > > > +       st = iio_priv(indio_dev);
> > > > +
> > > > +       indio_dev->info = &adf4377_info;
> > > > +       indio_dev->name = "adf4377";
> > > > +       indio_dev->channels = adf4377_channels;
> > > > +       indio_dev->num_channels = ARRAY_SIZE(adf4377_channels);
> > > > +
> > > > +       st->regmap = regmap;
> > > > +       st->spi = spi;
> > > > +       st->type = spi_get_device_id(spi)->driver_data;    
> > > 
> > > Hmm this is something that came up internally the other day. Are we
> > > guaranteed that this will always work? For OF I think it is but I'm
> > > not
> > > sure about ACPI? At first glance, it seems that it might be ok but
> > > I
> > > did not went too deep in the ACPI code.  
> > 
> > Better indeed to not assume it and indeed ACPI can't do this magic,
> > because
> > there isn't a match between the actual ACPI ID and the
> > spi_device_ids.
> > Not sure what it does with PRP0001 case (where it uses the
> > of_device_id table).  
> 
> Some comments/questions on the pattern:
> 
> > 
> > 
> >         st->type = device_get_match_id()->driver_data;  
> 
> Are we sure that device_get_match_id() (I guess you meant
> device_get_match_data()) cannot return NULL? I'm always a bit on the
> careful side with these dereferences...

Oops. Yes I got confused.  It can indeed return
NULL and when using right function we won't dereference it.
We'll just check st->type which will have been set NULL if that fails.

Jonathan
 
> 
> >         if (!st->type) {
> >                 const struct spi_device_id *id =
> > spi_get_device_id(spi);
> > 
> >                 if (!id)
> >                         return -EINVAL;
> > 
> >                 st->type = spi_get_device_id(spi)->driver_data;  
> 
> id->driver_data :)
> 
> >         }  
> 
> - Nuno Sá
> 
> 
> 
> 
> device_get_match_id()->driver_data;
> 
>