| 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: <20200725202733.GA946@kunai>
Date: Sat, 25 Jul 2020 22:27:33 +0200
From: Wolfram Sang <wsa@...nel.org>
To: Alain Volmat <alain.volmat@...com>
Cc: pierre-yves.mordret@...com, alexandre.torgue@...com,
linux-i2c@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
fabrice.gasnier@...com
Subject: Re: [PATCH v2 1/2] i2c: smbus: add core function handling SMBus
host-notify
Hi Alain,
some more comments for this one. I hope to come to a conclusion with Rob
regarding the binding for patch 2, so we are then ready to go.
On Fri, Jul 03, 2020 at 01:36:07PM +0200, Alain Volmat wrote:
> SMBus Host-Notify protocol, from the adapter point of view
> consist of receiving a message from a client, including the
> client address and some other data.
>
> It can be simply handled by creating a new slave device
> and registering a callback performing the parsing of the
> message received from the client.
>
> This commit introduces two new core functions
> * i2c_new_slave_host_notify_device
> * i2c_free_slave_host_notify_device
> that take care of registration of the new slave device and
> callback and will call i2c_handle_smbus_host_notify once a
> Host-Notify event is received.
>
> Signed-off-by: Alain Volmat <alain.volmat@...com>
> ---
> v2: identical to v1
>
> drivers/i2c/i2c-core-smbus.c | 114 +++++++++++++++++++++++++++++++++++++++++++
I came to the conclusion that this code should be in i2c-smbus.c.
Because it is SMBus only. I agree that the current code layout is
confusing. I will try to move the whole host-notify support to i2c-smbus
in the next cycle.
Yes, that means that one needs to select I2C_SMBUS in the config, too
(unless you want to 'select' it with your driver). But most people won't
need it so they can compile it away. This is what I2C_SMBUS is for.
> include/linux/i2c-smbus.h | 12 +++++
> 2 files changed, 126 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index f5c9787992e9..23ab9dc5ac85 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -715,3 +715,117 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
> }
> EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
> #endif
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +#define SMBUS_HOST_NOTIFY_LEN 3
> +struct i2c_slave_host_notify_status {
> + u8 index;
> + u8 addr;
> +};
> +
> +static int i2c_slave_host_notify_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct i2c_slave_host_notify_status *status = client->dev.platform_data;
> + int ret;
> +
Can't we simplify 'index' handling similar to the testunit driver...
> + switch (event) {
> + case I2C_SLAVE_WRITE_REQUESTED:
> + status->index = 0;
... by removing this line...
> + break;
> + case I2C_SLAVE_WRITE_RECEIVED:
> + /* We only retrieve the first byte received (addr)
> + * since there is currently no support to retrieve the data
> + * parameter from the client.
> + */
> + if (status->index == 0)
> + status->addr = *val;
> + if (status->index < U8_MAX)
> + status->index++;
> + break;
> + case I2C_SLAVE_STOP:
> + /* Handle host-notify if whole message received only */
> + if (status->index != SMBUS_HOST_NOTIFY_LEN) {
> + status->index = U8_MAX;
> + break;
> + }
> +
> + ret = i2c_handle_smbus_host_notify(client->adapter,
> + status->addr);
> + if (ret < 0)
> + return ret;
> + status->index = U8_MAX;
... and handling the logic here like:
+ if (status->index == SMBUS_HOST_NOTIFY_LEN)
+ i2c_handle_smbus_host_notify(client->adapter, status->addr);
+ status->index = 0;
Note that I2C_SLAVE_STOP doesn't return a retval, so we don't need to check
i2c_handle_smbus_host_notify().
> + break;
> + case I2C_SLAVE_READ_REQUESTED:
> + case I2C_SLAVE_READ_PROCESSED:
> + *val = 0xff;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * i2c_new_slave_host_notify_device - get a client for SMBus host-notify support
> + * @adapter: the target adapter
> + * Context: can sleep
> + *
> + * Setup handling of the SMBus host-notify protocol on a given I2C bus segment.
> + *
> + * Handling is done by creating a device and its callback and handling data
> + * received via the SMBus host-notify address (0x8)
> + *
> + * This returns the client, which should be ultimately freed using
> + * i2c_free_slave_host_notify_device(); or an ERRPTR to indicate an error.
> + */
> +struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter)
> +{
> + struct i2c_board_info host_notify_board_info = {
> + I2C_BOARD_INFO("smbus_host_notify", 0x08),
> + .flags = I2C_CLIENT_SLAVE,
> + };
> + struct i2c_slave_host_notify_status *status;
> + struct i2c_client *client;
> + int ret;
> +
> + status = kzalloc(sizeof(struct i2c_slave_host_notify_status),
> + GFP_KERNEL);
> + if (!status)
> + return ERR_PTR(-ENOMEM);
> + status->index = U8_MAX;
This line could go, too, if my simplification above works.
> +
> + host_notify_board_info.platform_data = status;
> +
> + client = i2c_new_client_device(adapter, &host_notify_board_info);
> + if (IS_ERR(client)) {
> + kfree(status);
> + return client;
> + }
> +
> + ret = i2c_slave_register(client, i2c_slave_host_notify_cb);
> + if (ret) {
> + i2c_unregister_device(client);
> + kfree(status);
> + return ERR_PTR(ret);
> + }
> +
> + return client;
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_slave_host_notify_device);
> +
> +/**
> + * i2c_free_slave_host_notify_device - free the client for SMBus host-notify
> + * support
> + * @client: the client to free
> + * Context: can sleep
> + *
> + * Free the i2c_client allocated via i2c_new_slave_host_notify_device
> + */
> +void i2c_free_slave_host_notify_device(struct i2c_client *client)
> +{
> + i2c_slave_unregister(client);
> + kfree(client->dev.platform_data);
> + i2c_unregister_device(client);
> +}
> +EXPORT_SYMBOL_GPL(i2c_free_slave_host_notify_device);
Sidenote: With my recent series "i2c: slave: improve sanity checks when
un-/registering" this code became NULL-safe (and IS_ERR safe, too).
> +#endif
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index 1e4e0de4ef8b..52e62f398f1c 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -38,6 +38,18 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
> return 0;
> }
> #endif
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter);
> +void i2c_free_slave_host_notify_device(struct i2c_client *client);
> +#else
> +static inline struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter)
> +{
> + return NULL;
return ERR_PTR(-ENOSYS);
As the docs say, this function either returns a valid client or an
ERR_PTR.
> +}
> +static inline void i2c_free_slave_host_notify_device(struct i2c_client *client)
> +{
> +}
> +#endif
>
> #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
> void i2c_register_spd(struct i2c_adapter *adap);
> --
> 2.7.4
>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists