| 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: <CA+U=DspTbh2Jp+51yDjeL23FE0LRocPS26+nFqQdSyVNCnB2mQ@mail.gmail.com>
Date: Mon, 28 Sep 2020 13:13:15 +0300
From: Alexandru Ardelean <ardeleanalex@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alexandru Ardelean <alexandru.ardelean@...log.com>,
linux-iio <linux-iio@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Lars-Peter Clausen <lars@...afoo.de>
Subject: Re: [RFC PATCH 0/5] iio: remove iio_buffer_set_attrs() and assign
buffer attrs during alloc
On Sat, Sep 26, 2020 at 7:20 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Fri, 25 Sep 2020 11:37:38 +0300
> Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
>
> > I've been mulling this over for a while, and I am still not 100% convinced
> > that this is the best approach, but this feels closer to something
> > correct.
>
> I'd not worry too much. It might not be perfect but it's also not hideous
> - though that won't stop me suggesting an alternative :)
>
> >
> > A few things about this patchset:
> > 1. it hasn't been tested; it just compiles
> > 2. there are some patches that have went out [from my side] in the last
> > few days that deal with trying to organize iio_buffer_set_attrs()
> > in order to get to this point [as-in: this patch-set]
> > 2a. the only reason patch [1] is in this set, is so that [5] applies
> > 2b. the main reason patch [2] is in this set, is so that [5] compiles
> > 3. This patch is only meant as RFC, as I'd like that some cleanup
> > patches go in before this set; this RFC is meant to provide an idea
> > of where I want to get at.
> >
> > This tries to address the issue with iio_buffer_set_attrs(), where a
> > driver needs to provide a reference to a buffer to assign extra sysfs
> > attributes.
> > Something like 'iio_buffer_set_attrs(indio_dev->buffer, <buffer_attrs>)'
> >
> > This works well for the 1 IIO buffer == 1 IIO device case.
> > The driver access the 'buffer' reference attached to the IIO device.
> >
> > But for a multiple IIO buffers per 1 IIO device, this is cumbersome, as
> > a driver would need to:
> > 1. allocate a set of buffers
> > 2. dig out of IIO the buffers to which it wants to assign extra sysfs
> > attributes
> >
> > The main change here is to move the attributes [usually HW FIFO
> > attributes] to become arguments of the devm_}iio_triggered_buffer_setup()
> > functions.
> > Other buffer allocation functions would need to do the same, but right
> > now, it's only these functions that assign extra buffer attributes.
>
> Probably worth stating here that we have a confusing mess of where
> these attributes are and will probably look long term to move them
> or at least think about doing so.
>
> The buffer directory in modern IIO isn't strictly matched to a hwfifo
> any more (it used to be - see sca3000 for the history - the model
> was not a nice solution, but it took a few years to figure that out).
> buffer/* represent a software buffer fed by the hardware fifo. There
> are potentially other software buffers being fed in the form of
> other consumers.
>
> The hardware fifo is a separate entity. However as Alex adds
> support for multiple streams of data (that may come from mulitple hardware
> fifos) we are going to still need a mapping of hardware buffer X
> to software buffer X. So separate entities, but with a one to one
> (technically many of the consumer side, but only one in the IIO
> userspace interface) mapping.
>
> Anyhow, Alex and others know this from previous discussions but perhaps others
> reading this don't hence I thought I'd mention it :)
>
> >
> > Some alternative ideas to doing this change [as-is]:
> > * move the attributes to 'struct iio_buffer_setup_ops' ; this has a
> > minimal patch/change-impact, but it doesn't feel correct
> > * introduce a new type called 'struct iio_trigerred_buffer_args' that
> > wrap all the triggered-buffer arguments into a single object; this is
> > still a big patch [like this], but future-wise, we would just be
> > extending that object [if more args are needed]; the main point
> > here is that right now 'devm_iio_triggered_buffer_setup()' has 6
> > arguments, which feels a bit much
>
> So here is another one:
> * Let us assume that we actually don't want to use this interface in new
> drivers because we decide to deprecate putting the hardware fifo stuff
> where it currently is. (big assumption - dependent on what userspace
> code is using this and it will take a while).
> * With that assumption, we could add a new function that takes the attrs
> e.g. iio_setup_triggered_buffer_ext()
> and only replace the instances where we need to provide the attrs.
>
Fine by me as well, to do a iio_setup_triggered_buffer_ext() that
holds the attrs as an extra arg.
I guess the thought [of doing it like that] didn't cross my mind, but
maybe also because I keep forgetting about the full context of the
buffer attributes.
That would convert "iio_setup_triggered_buffer()" into a wrapper for
iio_setup_triggered_buffer_ext() with NULL in place.
And is indeed a smaller patchset.
I'll send a V2 for this.
Maybe I'll wait until the at91-sama5d2_adc patch [1] is applied, or V2
will include a copy of that patch.
The cross_ec one was applied, so one less hurdle for this.
These cleanup patches [in various drivers] do help with making some of
the IIO core changes easier to grasp [for me particularly].
On a side-note, I was super happy when "scan_el_attrs" was dropped [2].
It's one less worry for this whole IIO buffer work.
[1] https://lore.kernel.org/linux-iio/20200926165549.23dc420b@archlinux/T/#t
[2] https://lore.kernel.org/linux-iio/20200412110253.52d93e71@archlinux/
> To be honest, as I said above, I'm not that bothered which route is
> taken.
>
> Jonathan
>
> >
> >
> > Alexandru Ardelean (5):
> > [1] iio: cros_ec: unify hw fifo attributes into the core file
> > [2] iio: buffer: dmaengine: unwrap the use of iio_buffer_set_attrs()
> > [3] iio: triggered-buffer: add sysfs buffer attributes as args for setup
> > [4] iio: buffer: extend arg list for {devm_}iio_triggered_buffer_setup()
> > [5] iio: remove iio_buffer_set_attrs() and assign buffer attrs during
> > alloc
> >
> > drivers/iio/accel/adxl372.c | 5 ++--
> > drivers/iio/accel/bma180.c | 2 +-
> > drivers/iio/accel/bma220_spi.c | 2 +-
> > drivers/iio/accel/bmc150-accel-core.c | 19 ++++++++------
> > drivers/iio/accel/cros_ec_accel_legacy.c | 2 +-
> > drivers/iio/accel/kxcjk-1013.c | 2 +-
> > drivers/iio/accel/kxsd9.c | 2 +-
> > drivers/iio/accel/mma7455_core.c | 2 +-
> > drivers/iio/accel/mma8452.c | 2 +-
> > drivers/iio/accel/mxc4005.c | 2 +-
> > drivers/iio/accel/st_accel_buffer.c | 2 +-
> > drivers/iio/accel/stk8312.c | 2 +-
> > drivers/iio/accel/stk8ba50.c | 2 +-
> > drivers/iio/adc/ad7266.c | 3 ++-
> > drivers/iio/adc/ad7298.c | 2 +-
> > drivers/iio/adc/ad7476.c | 2 +-
> > drivers/iio/adc/ad7606.c | 2 +-
> > drivers/iio/adc/ad7766.c | 2 +-
> > drivers/iio/adc/ad7768-1.c | 2 +-
> > drivers/iio/adc/ad7887.c | 2 +-
> > drivers/iio/adc/ad7923.c | 2 +-
> > drivers/iio/adc/ad799x.c | 2 +-
> > drivers/iio/adc/ad_sigma_delta.c | 2 +-
> > drivers/iio/adc/at91-sama5d2_adc.c | 25 +++++++++++--------
> > drivers/iio/adc/at91_adc.c | 2 +-
> > drivers/iio/adc/cc10001_adc.c | 2 +-
> > drivers/iio/adc/dln2-adc.c | 3 ++-
> > drivers/iio/adc/hx711.c | 3 ++-
> > drivers/iio/adc/max1027.c | 2 +-
> > drivers/iio/adc/max1118.c | 2 +-
> > drivers/iio/adc/max1363.c | 2 +-
> > drivers/iio/adc/mxs-lradc-adc.c | 2 +-
> > drivers/iio/adc/rockchip_saradc.c | 2 +-
> > drivers/iio/adc/stm32-adc.c | 2 +-
> > drivers/iio/adc/stm32-dfsdm-adc.c | 2 +-
> > drivers/iio/adc/ti-adc081c.c | 3 ++-
> > drivers/iio/adc/ti-adc0832.c | 2 +-
> > drivers/iio/adc/ti-adc084s021.c | 3 ++-
> > drivers/iio/adc/ti-adc108s102.c | 3 ++-
> > drivers/iio/adc/ti-adc12138.c | 2 +-
> > drivers/iio/adc/ti-adc161s626.c | 2 +-
> > drivers/iio/adc/ti-ads1015.c | 2 +-
> > drivers/iio/adc/ti-ads124s08.c | 3 ++-
> > drivers/iio/adc/ti-ads7950.c | 3 ++-
> > drivers/iio/adc/ti-ads8688.c | 3 ++-
> > drivers/iio/adc/ti-tlc4541.c | 2 +-
> > drivers/iio/adc/vf610_adc.c | 3 ++-
> > drivers/iio/adc/xilinx-xadc-core.c | 2 +-
> > .../buffer/industrialio-buffer-dmaengine.c | 3 +--
> > .../buffer/industrialio-triggered-buffer.c | 13 +++++++---
> > drivers/iio/chemical/atlas-sensor.c | 2 +-
> > drivers/iio/chemical/ccs811.c | 2 +-
> > drivers/iio/chemical/pms7003.c | 3 ++-
> > drivers/iio/chemical/scd30_core.c | 3 ++-
> > drivers/iio/chemical/sps30.c | 3 ++-
> > .../cros_ec_sensors/cros_ec_lid_angle.c | 5 ++--
> > .../common/cros_ec_sensors/cros_ec_sensors.c | 5 ++--
> > .../cros_ec_sensors/cros_ec_sensors_core.c | 16 +++++++++---
> > .../common/hid-sensors/hid-sensor-trigger.c | 19 ++++++--------
> > drivers/iio/gyro/adxrs290.c | 3 ++-
> > drivers/iio/gyro/bmg160_core.c | 2 +-
> > drivers/iio/gyro/fxas21002c_core.c | 3 ++-
> > drivers/iio/gyro/itg3200_buffer.c | 2 +-
> > drivers/iio/gyro/mpu3050-core.c | 2 +-
> > drivers/iio/gyro/st_gyro_buffer.c | 2 +-
> > drivers/iio/health/afe4403.c | 2 +-
> > drivers/iio/health/afe4404.c | 2 +-
> > drivers/iio/humidity/am2315.c | 2 +-
> > drivers/iio/humidity/hdc100x.c | 2 +-
> > drivers/iio/humidity/hts221_buffer.c | 2 +-
> > drivers/iio/imu/adis_buffer.c | 2 +-
> > drivers/iio/imu/bmi160/bmi160_core.c | 3 ++-
> > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 2 +-
> > drivers/iio/imu/kmx61.c | 4 +--
> > drivers/iio/industrialio-buffer.c | 12 ---------
> > drivers/iio/light/adjd_s311.c | 2 +-
> > drivers/iio/light/as73211.c | 3 ++-
> > drivers/iio/light/cros_ec_light_prox.c | 5 ++--
> > drivers/iio/light/gp2ap020a00f.c | 3 ++-
> > drivers/iio/light/isl29125.c | 2 +-
> > drivers/iio/light/ltr501.c | 2 +-
> > drivers/iio/light/max44000.c | 3 ++-
> > drivers/iio/light/rpr0521.c | 2 +-
> > drivers/iio/light/si1145.c | 2 +-
> > drivers/iio/light/st_uvis25_core.c | 2 +-
> > drivers/iio/light/tcs3414.c | 2 +-
> > drivers/iio/light/tcs3472.c | 2 +-
> > drivers/iio/light/vcnl4000.c | 3 ++-
> > drivers/iio/light/vcnl4035.c | 2 +-
> > drivers/iio/magnetometer/ak8974.c | 2 +-
> > drivers/iio/magnetometer/ak8975.c | 2 +-
> > drivers/iio/magnetometer/bmc150_magn.c | 2 +-
> > drivers/iio/magnetometer/hmc5843_core.c | 2 +-
> > drivers/iio/magnetometer/mag3110.c | 2 +-
> > drivers/iio/magnetometer/rm3100-core.c | 2 +-
> > drivers/iio/magnetometer/st_magn_buffer.c | 2 +-
> > drivers/iio/potentiostat/lmp91000.c | 2 +-
> > drivers/iio/pressure/cros_ec_baro.c | 5 ++--
> > drivers/iio/pressure/dlhl60d.c | 2 +-
> > drivers/iio/pressure/mpl3115.c | 2 +-
> > drivers/iio/pressure/ms5611_core.c | 2 +-
> > drivers/iio/pressure/st_pressure_buffer.c | 2 +-
> > drivers/iio/pressure/zpa2326.c | 2 +-
> > drivers/iio/proximity/as3935.c | 3 ++-
> > drivers/iio/proximity/isl29501.c | 2 +-
> > drivers/iio/proximity/mb1232.c | 3 ++-
> > .../iio/proximity/pulsedlight-lidar-lite-v2.c | 2 +-
> > drivers/iio/proximity/srf08.c | 3 ++-
> > drivers/iio/proximity/sx9310.c | 2 +-
> > drivers/iio/proximity/sx9500.c | 2 +-
> > drivers/iio/temperature/maxim_thermocouple.c | 3 ++-
> > include/linux/iio/buffer.h | 3 ---
> > .../linux/iio/common/cros_ec_sensors_core.h | 4 +--
> > include/linux/iio/triggered_buffer.h | 7 ++++--
> > 114 files changed, 198 insertions(+), 172 deletions(-)
> >
>
Powered by blists - more mailing lists