Re: [PATCH v11 2/7] mfd: Add driver for ASUS Transformer embedded controller

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Svyatoslav,

On Tue, Oct 13, 2026 at 08:34:58PM +0300, Svyatoslav Ryhel wrote:
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/array_size.h>
> +#include <linux/debugfs.h>

Unused include (left over after debugfs support was dropped).

> +/**
> + * struct asus_ec_chip_info
> + *
> + * @name: prefix associated with the EC
> + * @variant: id of programming model of EC
> + * @mode: state of Factory Mode bit in EC control register
> + * @read_size: size of block to read
> + * @keyboard_mask: mask of keyboard packets
> + */
> +struct asus_ec_chip_info {
> +	const char *name;
> +	enum asusec_variant variant;
> +	enum asusec_mode fmode;
> +	u8 read_size;
> +	u8 keyboard_mask;
> +};
> +
> +/**
> + * struct asus_ec_data
> + *
> + * @ec: public part shared with all cells (must be first)
> + * @ecreq_lock: prevents simultaneous access to EC
> + * @ecreq_gpio: EC request GPIO
> + * @client: pointer to EC's i2c_client
> + * @info: pointer to EC's version description
> + * @ec_buf: buffer for EC read
> + * @logging_disabled: flag disabling logging on reset events
> + */
> +struct asus_ec_data {
> +	struct asusec_core ec;
> +	struct mutex ecreq_lock;
> +	struct gpio_desc *ecreq_gpio;
> +	struct i2c_client *client;
> +	const struct asus_ec_chip_info *info;
> +	u8 ec_buf[ASUSEC_ENTRY_BUFSIZE];
> +};

scripts/kernel-doc warns on these two structs:
- In struct asus_ec_chip_info, the kerneldoc says @mode while the field is
  named fmode.
- In struct asus_ec_data, @logging_disabled no longer exists.

> +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> +			    u64 xor)
> +{
> +	struct dockram_ec_data *ddata = i2c_get_clientdata(client);
> +	u8 *buf = ddata->ctl_buf;
...
> +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> +{
> +	struct i2c_client *parent = to_i2c_client(dev);
> +	struct i2c_client *dockram;
> +	struct dockram_ec_data *ddata;
> +	int ret;
> +
> +	dockram = i2c_new_ancillary_device(parent, "dockram",
> +					   parent->addr + ASUSEC_DOCKRAM_OFFSET);
> +	if (IS_ERR(dockram))
> +		return dockram;
> +
> +	ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> +				       dockram);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ddata = devm_kzalloc(&dockram->dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	i2c_set_clientdata(dockram, ddata);
> +	mutex_init(&ddata->ctl_lock);
> +
> +	return dockram;
> +}

Attaching ddata to &dockram->dev via devm_kzalloc() and
i2c_set_clientdata() is fragile because dockram is bound to the I2C dummy
driver. If the dummy driver is unbound from dockram via sysfs, the driver
core frees &dockram->dev devres allocations and clears dockram's
driver_data to NULL, so the next call to asus_dockram_access_ctl() will
dereference NULL.

Instead of allocating a separate struct dockram_ec_data on the ancillary
i2c_client, could ctl_lock and ctl_buf live in struct asus_ec_data (or
struct asusec_core) and have asus_dockram_access_ctl() take struct
asusec_core *ec?

Thanks.

-- 
Dmitry