linux-kernel - Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts

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:   Mon, 17 Oct 2022 11:29:58 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Cosmin Tanislav <demonsingur@...il.com>
CC:     Nuno Sá <nuno.sa@...log.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        "Jonathan Cameron" <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts

On Mon, 17 Oct 2022 09:59:27 +0300
Cosmin Tanislav <demonsingur@...il.com> wrote:

> On 10/14/22 18:44, Jonathan Cameron wrote:
> > On Fri, 14 Oct 2022 15:37:24 +0300
> > Cosmin Tanislav <demonsingur@...il.com> wrote:
> >   
> >> From: Cosmin Tanislav <cosmin.tanislav@...log.com>
> >>
> >> Add support for the following parts:
> >>   * LTC2984
> >>   * LTC2986
> >>   * LTM2985
> >>
> >> The LTC2984 is a variant of the LTC2983 with EEPROM.
> >> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> >> EEPROM and support for active analog temperature sensors.
> >> The LTM2985 is software-compatible with the LTC2986.
> >>
> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@...log.com>  
> > 
> > ...
> > 
> > Hi Cosmin,
> > 
> > Looks good except, I think we are still in the position that
> > regmap for spi doesn't guarantee to bounce buffer the bulk accesses
> > (last time I checked it actually did do so, but before that it didn't
> > and there are obvious optimizations to take it back to not doing so -
> > IRC Mark Brown's answer was we shouldn't rely on it..)
> > 
> > Anyhow, the existing driver has instances of this so its no worse
> > but we should really clean those up.
> > 
> > Jonathan
> >   
> 
> I can submit another patch for it. Although I'm pretty sure that
> SPI regmap implementation doesn't need DMA safe access for it,
> as I checked when I wrote the code.

It doesn't today (last time I checked it didn't anyway and that matches
with what you found), but it did in the past and there are no guarantees
it won't require DMA safe buffers in the future.  I doubt I'll find the
reference but we had a discussion with Mark Brown about this for a similar
case a year or two back and he confirmed we should continue to assume
that DMA safe buffers should be used. 

Jonathan

> 
> >   
> >>   
> >> +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
> >> +			      unsigned int wait_time, unsigned int status_reg,
> >> +			      unsigned long status_fail_mask)
> >> +{
> >> +	__be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY);
> >> +	unsigned long time;
> >> +	unsigned int val;
> >> +	int ret;
> >> +
> >> +	ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval,
> >> +				sizeof(bval));  
> > 
> > SPI device and I was clearly dozing on existing driver but normally
> > we avoid assuming that regmap will always use a bounce buffer for bulk
> > accessors. Hence this should be a DMA safe buffer.
> > 
> >   
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	reinit_completion(&st->completion);
> >> +
> >> +	ret = regmap_write(st->regmap, LTC2983_STATUS_REG,
> >> +			   LTC2983_STATUS_START(true) | cmd);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	time = wait_for_completion_timeout(&st->completion,
> >> +					   msecs_to_jiffies(wait_time));
> >> +	if (!time) {
> >> +		dev_err(&st->spi->dev, "EEPROM command timed out\n");
> >> +		return -ETIMEDOUT;
> >> +	}
> >> +
> >> +	ret = regmap_read(st->regmap, status_reg, &val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (val & status_fail_mask) {
> >> +		dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +  
> >