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: <e07248e6-4987-b7a1-327a-7d30a3e5cdf5@axentia.se> Date: Wed, 13 Mar 2019 07:04:06 +0000 From: Peter Rosin <peda@...ntia.se> To: Greg Kroah-Hartman <gregkh@...uxfoundation.org> CC: Robert Shearman <robertshearman@...il.com>, Guenter Roeck <linux@...ck-us.net>, "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>, Wolfram Sang <wsa@...-dreams.de>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Robert Shearman <robert.shearman@....com> Subject: Re: [PATCH v4 3/3] i2c: mux: pca954x: allow management of device idle state via sysfs Hi Greg, I was wondering if you think the below ABI addition looks sane to you? Or, should I ask someone else for a tag? Cheers, Peter On 2019-02-28 12:43, Robert Shearman wrote: > From: Robert Shearman <robert.shearman@....com> > > The behaviour, by default, to not deselect after each transfer is > unsafe when there is a device with an address that conflicts with > another device on another mux on the same parent bus, and it > may not be convenient to use devicetree to set the deselect mux, > e.g. when running on x86_64 when ACPI is used to discover most of the > device hierarchy. > > Therefore, provide the ability to set the idle state behaviour using a > new sysfs file, idle_state as a complement to the method of > instantiating the device via sysfs. The possible behaviours are > disconnect, i.e. to deselect all channels from the mux, as-is (the > default), i.e. leave the last channel selected, and set a > predetermined channel. > > The current behaviour of leaving the channel as-is after each > transaction is preserved. > > Signed-off-by: Robert Shearman <robert.shearman@....com> > --- > .../ABI/testing/sysfs-bus-i2c-devices-pca954x | 20 +++++ > drivers/i2c/muxes/i2c-mux-pca954x.c | 85 +++++++++++++++++-- > 2 files changed, 97 insertions(+), 8 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x > > diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x b/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x > new file mode 100644 > index 000000000000..0b0de8cd0d13 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x > @@ -0,0 +1,20 @@ > +What: /sys/bus/i2c/.../idle_state > +Date: January 2019 > +KernelVersion: 5.2 > +Contact: Robert Shearman <robert.shearman@....com> > +Description: > + Value that exists only for mux devices that can be > + written to control the behaviour of the multiplexer on > + idle. Possible values: > + -2 - disconnect on idle, i.e. deselect the last used > + channel, which is useful when there is a device > + with an address that conflicts with another > + device on another mux on the same parent bus. > + -1 - leave the mux as-is, which is the most optimal > + setting in terms of I2C operations and is the > + default mode. > + 0..<nchans> - set the mux to a predetermined channel, > + which is useful if there is one channel that is > + used almost always, and you want to reduce the > + latency for normal operations after rare > + transactions on other channels > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > index e32fef560684..923aa3a5a3dc 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -49,6 +49,7 @@ > #include <linux/pm.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > +#include <dt-bindings/mux/mux.h> > > #define PCA954X_MAX_NCHANS 8 > > @@ -84,7 +85,9 @@ struct pca954x { > const struct chip_desc *chip; > > u8 last_chan; /* last register value */ > - u8 deselect; > + /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */ > + s8 idle_state; > + > struct i2c_client *client; > > struct irq_domain *irq; > @@ -253,15 +256,71 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan) > { > struct pca954x *data = i2c_mux_priv(muxc); > struct i2c_client *client = data->client; > + s8 idle_state; > + > + idle_state = READ_ONCE(data->idle_state); > + if (idle_state >= 0) > + /* Set the mux back to a predetermined channel */ > + return pca954x_select_chan(muxc, idle_state); > + > + if (idle_state == MUX_IDLE_DISCONNECT) { > + /* Deselect active channel */ > + data->last_chan = 0; > + return pca954x_reg_write(muxc->parent, client, > + data->last_chan); > + } > > - if (!(data->deselect & (1 << chan))) > - return 0; > + /* otherwise leave as-is */ > > - /* Deselect active channel */ > - data->last_chan = 0; > - return pca954x_reg_write(muxc->parent, client, data->last_chan); > + return 0; > +} > + > +static ssize_t idle_state_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + struct pca954x *data = i2c_mux_priv(muxc); > + > + return sprintf(buf, "%d\n", READ_ONCE(data->idle_state)); > +} > + > +static ssize_t idle_state_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + struct pca954x *data = i2c_mux_priv(muxc); > + int val; > + int ret; > + > + ret = kstrtoint(buf, 0, &val); > + if (ret < 0) > + return ret; > + > + if (val != MUX_IDLE_AS_IS && val != MUX_IDLE_DISCONNECT && > + (val < 0 || val >= data->chip->nchans)) > + return -EINVAL; > + > + i2c_lock_bus(muxc->parent, I2C_LOCK_SEGMENT); > + > + WRITE_ONCE(data->idle_state, val); > + /* > + * Set the mux into a state consistent with the new > + * idle_state. > + */ > + if (data->last_chan || val != MUX_IDLE_DISCONNECT) > + ret = pca954x_deselect_mux(muxc, 0); > + > + i2c_unlock_bus(muxc->parent, I2C_LOCK_SEGMENT); > + > + return ret < 0 ? ret : count; > } > > +static DEVICE_ATTR_RW(idle_state); > + > static irqreturn_t pca954x_irq_handler(int irq, void *dev_id) > { > struct pca954x *data = dev_id; > @@ -328,8 +387,11 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc) > static void pca954x_cleanup(struct i2c_mux_core *muxc) > { > struct pca954x *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > int c, irq; > > + device_remove_file(&client->dev, &dev_attr_idle_state); > + > if (data->irq) { > for (c = 0; c < data->chip->nchans; c++) { > irq = irq_find_mapping(data->irq, c); > @@ -410,9 +472,12 @@ static int pca954x_probe(struct i2c_client *client, > } > > data->last_chan = 0; /* force the first selection */ > + data->idle_state = MUX_IDLE_AS_IS; > > idle_disconnect_dt = np && > of_property_read_bool(np, "i2c-mux-idle-disconnect"); > + if (idle_disconnect_dt) > + data->idle_state = MUX_IDLE_DISCONNECT; > > ret = pca954x_irq_setup(muxc); > if (ret) > @@ -420,8 +485,6 @@ static int pca954x_probe(struct i2c_client *client, > > /* Now create an adapter for each channel */ > for (num = 0; num < data->chip->nchans; num++) { > - data->deselect |= idle_disconnect_dt << num; > - > ret = i2c_mux_add_adapter(muxc, 0, num, 0); > if (ret) > goto fail_cleanup; > @@ -436,6 +499,12 @@ static int pca954x_probe(struct i2c_client *client, > goto fail_cleanup; > } > > + /* > + * The attr probably isn't going to be needed in most cases, > + * so don't fail completely on error. > + */ > + device_create_file(dev, &dev_attr_idle_state); > + > dev_info(dev, "registered %d multiplexed busses for I2C %s %s\n", > num, data->chip->muxtype == pca954x_ismux > ? "mux" : "switch", client->name); >
Powered by blists - more mailing lists