linux-kernel - Re: [PATCH v3] iio: adc: pac1921: add ACPI support to Microchip pac1921.

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: <b068aa0765abc36890b3d88e8a71234a9e47be74.camel@microchip.com>
Date: Tue, 15 Oct 2024 08:02:31 +0000
From: <Victor.Duicu@...rochip.com>
To: <jic23@...nel.org>, <matteomartelli3@...il.com>
CC: <Marius.Cristea@...rochip.com>, <lars@...afoo.de>,
	<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v3] iio: adc: pac1921: add ACPI support to Microchip
 pac1921.

On Mon, 2024-10-14 at 19:46 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> > 
> > > > > +static int pac1921_parse_of_fw(struct i2c_client *client,
> > > > > struct
> > > > > pac1921_priv *priv,
> > > > > +                              struct iio_dev *indio_dev)
> > > > > +{
> > > > > +       int ret;
> > > > > +       struct device *dev = &client->dev;
> > > > > +       u64 temp;
> > > > > +
> > > > > +       ret = device_property_read_u64(dev, "shunt-resistor-
> > > > > micro-
> > > > > ohms", &temp);
> > > > 
> > > > Since the driver would discard a value out of INT boundaries, I
> > > > don't
> > > > see the
> > > > need to read a value larger than u32 that would be discarded
> > > > anyway.
> > > > To my
> > > > understanding, device_property_read_u32() should fail for an
> > > > overflowing value
> > > > thus I would keep device_property_read_u32() here, and at that
> > > > point
> > > > the temp
> > > > var would not be necessary as well. I think it would also help
> > > > to
> > > > keep the patch
> > > > diff confined in the ACPI extension context.
> > > 
> > > If the value in .dtso is greater than 32b, at compilation it will
> > > be
> > > truncated, and the incorrect value will be accepted by the
> > > driver. By
> > > adding "/bits/ 64" in the devicetree to shunt resistor the value
> > > will
> > > not be truncated. This way values on 32b and 64b can be read
> > > correctly.
> > > 
> > 
> > I see your point but if I understand this correctly with this
> > change the
> > shunt-resistor-micro-ohms field in the DT should always be
> > specified
> > with /bits/ 64, even for values in 32bit boundaries. I might be
> > wrong
> > but this looks like something that should be documented in
> > Documentation/devicetree/bindings, especially since all the other
> > shunt-resistor-micro-ohms instances look to be interpreted as u32.
> > Also, I think that such change would fit better in a different
> > patch as
> > it is not related to the introduction of ACPI support.
> 
> I'll ask the silly question. How big a shunt resistor do you have?
> If it's necessary to change them over that is fine but if that means
> existing dt is wrong, then you'd need to maintain compatibility.
> So test with a 32 bit dt value and 64 bit. Probably need to try
> 64 bit and if it fails try 32 bits.

The maximum resistor we use is 4K. I agree now that it is unnecessary
to change the devicetree to read 64b values. I will undo those changes
and read only 32b values.

> 
> > 
> > > > > +
> > > > > +       if (ret)
> > > > > +               return dev_err_probe(dev, ret,
> > > > > +                                    "Cannot read shunt
> > > > > resistor
> > > > > property\n");
> > > > > +
> > > > > +       if (pac1921_shunt_is_valid(temp))
> > > > > +               return dev_err_probe(dev, -EINVAL, "Invalid
> > > > > shunt
> > > > > resistor: %u\n",
> > > > > +                                    priv->rshunt_uohm);
> > > > 
> > > > The error should be returned when the shunt is NOT valid.
> > > > 
> > > > > +
> > > > > +       priv->rshunt_uohm = (u32)temp;
> > > > 
> > > > The temp var should not be necessary if switching back to
> > > > device_property_read_u32(),
> > > > otherwise I would remove the unnecessary explicit cast for the
> > > > above
> > > > reason.
> > > > 
> > > > > +       pac1921_calc_current_scales(priv);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > 
> > Thanks,
> > Matteo Martelli

Thank you,
Victor Duicu