On 26/05/18 03:26PM, Rodrigo Alencar wrote:
> On 26/05/17 06:56PM, [email protected] wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider:
> > - [Medium] Exposing the hierarchical relationship via channel labels
> > creates an ambiguous UAPI for userspace.
> > --
> >
> > commit 64c6f961f1d7645c58f732e4bd4c11200b999e93
> > Author: Rodrigo Alencar <[email protected]>
> >
> > iio: core: add hierarchical channel relationships
> >
> > This commit introduces a parent-child relationship between IIO channels.
> > It adds a parent pointer to the iio_chan_spec struct and exposes this
> > relationship to userspace through a new "parent" sysfs attribute that
> > returns the parent channel's label.
> >
> > > diff --git a/drivers/iio/industrialio-core.c
> > > b/drivers/iio/industrialio-core.c
> > > index 5c8404efd0a56..348ac7a597385 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> >
> > [ ... ]
> >
> > > @@ -1263,6 +1271,31 @@ static int iio_device_add_channel_label(struct
> > > iio_dev *indio_dev,
> > > return 1;
> > > }
> > >
> > > +static int iio_device_add_channel_parent(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan)
> > > +{
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > + int ret;
> > > +
> > > + if (!chan->parent || (!indio_dev->info->read_label &&
> > > + !chan->parent->extend_name))
> > > + return 0;
> >
> > Is relying on channel labels robust enough for programmatic parent
> > resolution?
> >
> > IIO channel labels are optional, human-readable descriptors and are not
> > guaranteed by the core to be unique. If multiple channels within a device
> > share the same label, how can userspace definitively resolve which specific
> > channel is the parent?
> >
> > Also, if a driver establishes a hierarchy where chan->parent is set, but the
> > parent channel lacks a label, this attribute is silently omitted, hiding the
> > relationship entirely.
> >
> > Would it be more reliable to expose a definitive, unique identifier (such as
> > the parent channel's sysfs prefix or index) rather than relying on an
> > optional
> > descriptive string?
>
> A bit impressive that sashiko can comment on that architecture level.
>
> Those are all good points. I suppose I went for this because otherwise it
> would
> require refactoring core code to get the needed parts from
> __iio_device_attr_init()
> that gets you the channel name without the attribute part.
>
> Maybe we should have a function that outputs a channel name into a buffer
> based on
> __iio_device_attr_init() behavior, and that could be reused.
This is the function with behavior extracted from __iio_device_attr_init():
static int __iio_chan_prefix_emit(const struct iio_chan_spec *chan,
enum iio_shared_by shared_by,
char *buf, size_t len)
{
const char *dir = iio_direction[chan->output];
const char *type = iio_chan_type_name_spec[chan->type];
int n = 0;
switch (shared_by) {
case IIO_SHARED_BY_ALL:
break;
case IIO_SHARED_BY_DIR:
n = scnprintf(buf, len, "%s", dir);
break;
case IIO_SHARED_BY_TYPE:
n = scnprintf(buf, len, "%s_%s", dir, type);
if (chan->differential)
n += scnprintf(buf + n, len - n, "-%s", type);
break;
case IIO_SEPARATE:
if (chan->indexed) {
n = scnprintf(buf, len, "%s_%s%d", dir, type,
chan->channel);
if (chan->differential)
n += scnprintf(buf + n, len - n,
"-%s%d", type,
chan->channel2);
} else {
if (chan->differential) {
WARN(1, "Differential channels must be
indexed\n");
return -EINVAL;
}
n = scnprintf(buf, len, "%s_%s", dir, type);
}
if (chan->modified) {
if (chan->differential) {
WARN(1, "Differential channels can not
have modifier\n");
return -EINVAL;
}
n += scnprintf(buf + n, len - n, "_%s",
iio_modifier_names[chan->channel2]);
}
if (chan->extend_name)
n += scnprintf(buf + n, len - n, "_%s",
chan->extend_name);
break;
}
return n;
}
I think it is clear and reusable.
> > > +
> > > + ret = __iio_add_chan_devattr("parent",
> > > + chan,
> > > + &iio_read_channel_parent,
> > > + NULL,
> > > + 0,
> > > + IIO_SEPARATE,
> > > + &indio_dev->dev,
> > > + NULL,
> > > + &iio_dev_opaque->channel_attr_list);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return 1;
> > > +}
> >
> > --
> > Sashiko AI review ยท
> > https://sashiko.dev/#/patchset/[email protected]?part=3
--
Kind regards,
Rodrigo Alencar
