| 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
| ||
|
Message-ID: <aSSm3JMY3DSg1Nns@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 20:41:32 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver
On Mon, Nov 24, 2025 at 07:29:06PM +0200, Petre Rodan wrote:
> thank you for the review.
You're welcome.
> On Mon, Nov 24, 2025 at 01:48:08PM +0200, Andy Shevchenko wrote:
> > On Sat, Nov 22, 2025 at 11:42:45PM +0200, Petre Rodan wrote:
[..]
> > > +/*
> > > + * transfer function A: 10% to 90% of 2^24
> >
> > Too many spaces, also this may be a one-line comment.
>
> it was intentional to have the comment multiline.
> in case we need to add additional transfer functions in the future for
> compatible ICs the diff will be a few lines smaller.
This is just a comment, won't be a big churn to change (note, it's different
to the cases of trailing commas where it has more significant impact).
> > > + */
...
> > > +enum abp2_variants {
> >
> > Why explicit assignments? Is it related to some HW register?
>
> no registers, I just need to ensure the two arrays
>
> static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
> [ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA", ..
>
> static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
> [ABP2001BA] = { .pmin = 0, .pmax = 100000 },
> [ABP21_6BA] = { .pmin = 0, .pmax = 160000 }, ..
>
> keep being consistent and are resistant to later editing.
So, if it's pure software numbering, just drop assignments in the enum.
> I feel like I had a better implementation two years ago when I used a single
> struct containing all this information and had a custom/NIH search function,
> but you kindly asked me [1] to use device_property_match_property_string()
> instead and split my single struct into this three parts mess.
Yes, and that still stays.
> [1] https://lore.kernel.org/linux-iio/ZWcUPkzfGqxYsysp@smile.fi.intel.com/
> > Can be done easier with a macro with more robustness against typos:
> >
> > #define ABP2_VARIANT(v) [ABP2 ## v] = ##v
> >
> > static const char * const abp2_triplet_variants[] = {
> > ABP2_VARIANT(001BA), ABP2_VARIANT(1_6BA), ABP2_VARIANT(2_5BA), ABP2_VARIANT(004BA),
> > ...
> > };
> >
> > but this will loose the possibility to make '.' in the name. Up to you.
>
> thanks, but I need '.' in the name. the dot is used in the part number (and
> thus in the pressure triplet).
OK.
...
> > > + if (data->irq > 0) {
> > > + ret = wait_for_completion_timeout(&data->completion, HZ);
> >
> > Where is HZ defined? Include that.
> >
> > > + if (!ret) {
> > > + dev_err(dev, "timeout waiting for EOC interrupt\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > + } else
> > > + fsleep(5000);
> >
> > Better to have 5 * USEC_PER_MSEC. Also missed comment why this long delay
> > is needed (will require time.h).
> >
> > Missed {} as well.
>
> I'm not sure I understand where are braces needed/not needed in this context.
It's according to the Coding Style. If one branch needs that, the second must
be on par.
...
> > > + /*
> > > + * status byte flags
> > > + * bit7 SANITY_CHK - must always be 0
> > > + * bit6 ABP2_ST_POWER - 1 if device is powered
> > > + * bit5 ABP2_ST_BUSY - 1 if device has no new conversion ready
> > > + * bit4 SANITY_CHK - must always be 0
> > > + * bit3 SANITY_CHK - must always be 0
> > > + * bit2 MEMORY_ERR - 1 if integrity test has failed
> > > + * bit1 SANITY_CHK - must always be 0
> > > + * bit0 MATH_ERR - 1 during internal math saturation err
> > > + */
> > > +
> > > + if (data->buffer[0] == (ABP2_ST_POWER | ABP2_ST_BUSY))
> > > + return -ETIMEDOUT;
-EBUSY as I read the comment above.
> > > + if (data->buffer[0] != ABP2_ST_POWER) {
> > > + dev_err(data->dev,
> > > + "unexpected status byte 0x%02x\n", data->buffer[0]);
> > > + return -ETIMEDOUT;
-EIO
> > > + }
> >
> > I am not sure the chosen error code in both cases is good enough.
>
> I'm open to recommendations on better error codes.
See below.
> first error: chip reports it's busy 5ms after start conversion command. based
> on the datasheet the conversion should have been ready at this point in time.
> this sounds to me like a timeout error.
I think EBUSY consistent with the comment given above in the code.
> second error: status byte contains unexpected flags being set - either an
> internal error - see table above or a bus read error. yes, timeout is not
> good here but what should it be?
>
> I'm using two conditionals because I want to log only invalid statuses and
> ignore simple 'device busy' errors.
You know the HW much better than me, so I proposed on the context I see in the
code, you may take the advice or do differently, but -ETIMEDOUT in these cases
seems to me not a fit.
...
> > > +struct abp2_ops {
> > > + int (*init)(struct device *dev);
> > > + int (*read)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> > > + int (*write)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> >
> > What is the meaning of const for the POD type parameters? I mean this gives
> > really a little protection if any. I do not see a point here to have them being const.
>
> I read a few books about C programming a few decades back and there was a
> consensus on using const in function prototypes wherever a parameter was
> supposed to not be changed. of course it's not bulletproof, but why do you
> feel I should stop following that advice for functions that are not tied to
> any pre-existing kernel API?
This is quite good advice for the pointers to the arrays and / or structures,
but for simple numbers it makes a little sense inside one small driver.
(Although, it has more sense in the generic libraries, but still.)
> > > +int abp2_common_probe(struct device *dev, const struct abp2_ops *ops, int irq);
You see, here I haven't commented anything as const qualifier for ops here is
very good and should be present like you have done already.
> > > +#endif
...
> > > +static int abp2_i2c_init(struct device *dev)
> > > +{
> > > + return 0;
> > > +}
> >
> > Is this stub required?
>
> do I have a 100% guarantee that the kernel will not try to execute a null
> pointer function in abp2_common_probe()?
Depends on what you do with this. If you do not check at the caller, then yes,
stub is needed, but in kernel we usually do
if (...->init) {
ret = ->init(...);
...
}
> ret = data->ops->init(data->dev); // needed only for SPI.
>
> later edit:
> since I will remove devm_kzalloc(), the _init will probably go away entirely
> together with the stub.
OK.
...
> > > +static int abp2_i2c_read(struct abp2_data *data, const u8 unused, const u8 cnt)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(data->dev);
> > > + int ret;
> > > +
> > > + if (cnt > ABP2_MEASUREMENT_RD_SIZE)
> > > + return -EOVERFLOW;
> > > +
> > > + ret = i2c_master_recv(client, data->buffer, cnt);
> > > + if (ret < 0)
> > > + return ret;
> >
> > > + else if (ret != cnt)
> >
> > Redundant 'else'.
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
>
> are you implying that __i2c_transfer() errors out if the number of bytes transfered is not cnt?
git log --grep "redundant 'else'"
will answer to this question.
[..]
> > So, why can't regmap SPI be used?
>
> there are no registers, no memory map, just a 'start conversion' and the
> equivalent of a 'read conversion' command.
> any reason one would use the regmap API in this case?
At bare minimum the commit message should have a justification for the choice
explaining all this. Ideally, try to find a way how to use regmap API. We have
several weeks of time for this exercise.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists