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

On Sat, 2024-10-12 at 12:05 +0200, Matteo Martelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Quoting victor.duicu@...rochip.com (2024-10-11 15:44:54)
> > From: Victor Duicu <victor.duicu@...rochip.com>
> > 
> > This patch implements ACPI support to Microchip pac1921.
> > The driver can read shunt resistor value and label from ACPI table.
> > 
> > The patch was tested on a minnowboard(64b) and sama5(32b).
> > In order to avoid overflow when reading 64b values from ACPi table
> > or
> > devicetree it is necessary:
> > - the revision of .dsl file must be 2 or greater to enable 64b
> > arithmetic.
> > - the shunt resistor variable in devicetree must have the prefix
> > "/bits/ 64".
> > 
> > Differences related to previous versions:
> > v3:
> > - simplify and make inline function pac1921_shunt_is_valid. Make
> > argument u64.
> > - fix link to DSM documentation.
> > - in pac1921_match_acpi_device and pac1921_parse_of_fw, the shunt
> > value is
> > read as u64.
> > - in pac1921_parse_of_fw remove code for reading label value from
> > devicetree.
> > - in pac1921_write_shunt_resistor cast the multiply result to u64
> > in order
> > to fix overflow.
> > 
> > v2:
> > - remove name variable from priv. Driver reads label attribute with
> > sysfs.
> > - define pac1921_shunt_is_valid function.
> > - move default assignments in pac1921_probe to original position.
> > - roll back coding style changes.
> > - add documentation for DSM(the linked document was used as
> > reference).
> > - remove acpi_match_device in pac1921_match_acpi_device.
> > - remove unnecessary null assignment and comment.
> > - change name of function pac1921_match_of_device to
> > pac1921_parse_of_fw.
> > 
> > v1:
> > - initial version for review.
> > 
> > Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
> > ---
> >  drivers/iio/adc/pac1921.c | 106 +++++++++++++++++++++++++++++++++-
> > ----
> >  1 file changed, 93 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > index 567279664e74..01c5eceab0be 100644
> > --- a/drivers/iio/adc/pac1921.c
> > +++ b/drivers/iio/adc/pac1921.c
> > @@ -67,6 +67,10 @@ enum pac1921_mxsl {
> >  #define PAC1921_DEFAULT_DI_GAIN                0 /* 2^(value): 1x
> > gain (HW default) */
> >  #define PAC1921_DEFAULT_NUM_SAMPLES    0 /* 2^(value): 1 sample
> > (HW default) */
> > 
> > +#define PAC1921_ACPI_GET_UOHMS_VALS             0
> > +#define PAC1921_ACPI_GET_LABEL                 1
> > +#define PAC1921_DSM_UUID                        "f7bb9932-86ee-
> > 4516-a236-7a7a742e55cb"
> > +
> >  /*
> >   * Pre-computed scale factors for BUS voltage
> >   * format: IIO_VAL_INT_PLUS_NANO
> > @@ -204,6 +208,11 @@ struct pac1921_priv {
> >         } scan;
> >  };
> > 
> > +static inline bool pac1921_shunt_is_valid(u64 shunt_val)
> > +{
> > +       return (shunt_val == 0 || shunt_val > INT_MAX);
> > +}
> > +
> 
> It's very confusing that this returns true when the shunt is NOT
> valid. I would
> either negate the return value or change the name.
> 
> >  /*
> >   * Check if first integration after configuration update has
> > completed.
> >   *
> > @@ -792,13 +801,13 @@ static ssize_t
> > pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
> >         if (ret)
> >                 return ret;
> > 
> > -       rshunt_uohm = val * MICRO + val_fract;
> > -       if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> > +       rshunt_uohm = (u64)val * MICRO + val_fract;
> 
> In commit a9bb0610b2fa ("iio: pac1921: remove unnecessary explicit
> casts"),
> unnecessary explicit casts had been removed since it seems the
> preferred
> approach in order to improve readability. This (u64)val cast seems
> unnecessary
> as well thus I would keep the expression without it.

While testing on SamA5 board , the multiplication between val and MICRO
can overflow when val is greater than INT_MAX. The cast to (u64) is
necessary to correctly calculate the new shunt value.

> > +       if (pac1921_shunt_is_valid(rshunt_uohm))
> >                 return -EINVAL;
> 
> The error should be returned when the shunt is NOT valid.
> 
> > 
> >         guard(mutex)(&priv->lock);
> > 
> > -       priv->rshunt_uohm = rshunt_uohm;
> > +       priv->rshunt_uohm = (u32)rshunt_uohm;
> 
> I would remove the unnecessary explicit cast for the above reason.
> > 
> >         pac1921_calc_current_scales(priv);
> > 
> > @@ -1150,6 +1159,74 @@ static void pac1921_regulator_disable(void
> > *data)
> >         regulator_disable(regulator);
> >  }
> > 
> > +/*
> > + * documentation related to the ACPI device definition
> > + *
> > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
> > + */
> > +static int pac1921_match_acpi_device(struct i2c_client *client,
> > struct pac1921_priv *priv,
> > +                                    struct iio_dev *indio_dev)
> > +{
> > +       acpi_handle handle;
> > +       union acpi_object *rez;
> > +       guid_t guid;
> > +       char *label;
> > +       u64 temp;
> > +
> > +       guid_parse(PAC1921_DSM_UUID, &guid);
> > +       handle = ACPI_HANDLE(&client->dev);
> > +
> > +       rez = acpi_evaluate_dsm(handle, &guid, 1,
> > PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> > +       if (!rez)
> > +               return dev_err_probe(&client->dev, -EINVAL,
> > +                                    "Could not read shunt from
> > ACPI table\n");
> > +
> > +       temp = rez->package.elements[0].integer.value;
> > +       ACPI_FREE(rez);
> > +
> > +       if (pac1921_shunt_is_valid(temp))
> > +               return dev_err_probe(&client->dev, -EINVAL,
> > "Invalid shunt resistor\n");
> 
> The error should be returned when the shunt is NOT valid.
> 
> > +
> > +       priv->rshunt_uohm = temp;
> > +       pac1921_calc_current_scales(priv);
> > +
> > +       rez = acpi_evaluate_dsm(handle, &guid, 1,
> > PAC1921_ACPI_GET_LABEL, NULL);
> > +       if (!rez)
> > +               return dev_err_probe(&client->dev, -EINVAL,
> > +                                    "Could not read label from
> > ACPI table\n");
> > +
> > +       label = devm_kmemdup(&client->dev, rez->package.elements-
> > >string.pointer,
> > +                            (size_t)rez->package.elements-
> > >string.length + 1,
> > +                            GFP_KERNEL);
> > +       label[rez->package.elements->string.length] = '\0';
> > +       indio_dev->label = label;
> > +       ACPI_FREE(rez);
> > +
> > +       return 0;
> > +}
> > +
> > +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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ