linux-kernel - Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Dec 2023 18:24:53 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Petre Rodan <petre.rodan@...dimension.ro>
CC:     Jonathan Cameron <jic23@...nel.org>,
        <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        Andreas Klinger <ak@...klinger.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC
 series pressure sensors

> > > +	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE];  
> > 
> > This is used for SPI transfers so should be DMA safe. It's not currently.
> > Look at how IIO_DMA_MINALIGN is used in other drivers to ensure there is
> > no unsafe sharing of cachelines.
> > 
> > On some architectures this is fixed by the stuff that bounces all small transfers
> > but I don't think that is universal yet.  If you want more info find the talk
> > by Wolfram Sang from a few years ago an ELCE on I2C DMA safe buffers.  
> 
> that was a nice rabbit hole, thanks for the pointer.

:) 

> 
> now, based on [2] I will skip explicit i2c dma-related code since my requests
> are 4 bytes long. according to the document, any i2c xfer below 8bytes is not
> worth the overhead.
> 
> [2] https://www.kernel.org/doc/html/latest/i2c/dma-considerations.html
> 
> > > +static int hsc_spi_probe(struct spi_device *spi)
> > > +{
> > > +	struct iio_dev *indio_dev;
> > > +	struct hsc_data *hsc;
> > > +	struct device *dev = &spi->dev;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	hsc = iio_priv(indio_dev);
> > > +	hsc->xfer = hsc_spi_xfer;  
> > 
> > Also, pass the callback and spi->dev into hsc probe. Easy to use
> > a container_of() to get back to the struct spi_device *spi  
> 
> I'd rather simply pass along the client struct.
> 

I don't like the fact it has to be a void *

The core code has no idea what is in there.  At least we constraint it
somewhat with a struct device.

If you want to use a union of the possible types, that would also be fine.

> > > +	hsc->client = spi;
> > > +
> > > +	return hsc_probe(indio_dev, &spi->dev, spi_get_device_id(spi)->name,
> > > +			 spi_get_device_id(spi)->driver_data);  
> > Don't use anything form spi_get_device_id()
> > 
> > Name is a fixed string currently so pass that directly.
> > For driver data, there isn't any yet but if there were use
> > spi_get_device_match_data() and make sure to provide the data in all the
> > id tables.  That function will search the firmware ones first then call
> > back to the spi specific varient.  
> 
> along the way driver_data became redundant, so it was removed from the function
> prototype.
> 
> best regards,
> peter
> 
>