Re: [PATCH v5 4/7] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon <boris.brezillon@collabora.com>]

On Sat, 22 Jun 2019 21:55:02 +0100
Przemyslaw Gaj <pgaj@cadence.com> wrote:


> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 86b7b44..cdfc5bf 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type;
>  
>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> +void i3c_bus_maintenance_lock(struct i3c_bus *bus);
> +void i3c_bus_maintenance_unlock(struct i3c_bus *bus);

I don't think you need to expose the maintenance lock helpers, looks
like they're only used in master.c.

> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master);
> +
>  
>  int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>  				 struct i3c_priv_xfer *xfers,
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index cbace14..3b44e66 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -93,6 +93,18 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
>  	up_read(&bus->lock);
>  }
>  
> +/*
> + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal
> + * operation
> + *
> + * Should be called when a maintenance operation is done and normal
> + * operation is planned. See i3c_bus_maintenance_lock() and
> + * i3c_bus_normaluse_lock() for more details.
> + */
> +static void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus)
> +{
> +	downgrade_write(&bus->lock);
> +}

Missing blank line.

>  static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev)
>  {
>  	return container_of(dev, struct i3c_master_controller, dev);
> @@ -341,6 +353,22 @@ static int i3c_device_probe(struct device *dev)
>  	return driver->probe(i3cdev);
>  }
>  
> +static int
> +i3c_master_enable_mr_events_locked(struct i3c_master_controller *master)
> +{
> +	if (!master->ops->enable_mr_events)
> +		return -ENOTSUPP;
> +
> +	return master->ops->enable_mr_events(master);
> +}
> +
> +static void i3c_master_disable_mr_events(struct i3c_master_controller *master)
> +{
> +	if (!master->ops->disable_mr_events)
> +		return;
> +
> +	master->ops->disable_mr_events(master);
> +}

Missing blank line.

>  static int i3c_device_remove(struct device *dev)
>  {
>  	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> @@ -462,6 +490,42 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
>  	return 0;
>  }
>  
> +static int
> +i3c_master_request_mastership_locked(struct i3c_master_controller *master)
> +{
> +	if (WARN_ON(master->init_done &&
> +	    !rwsem_is_locked(&master->bus.lock)))

Hm, that looks suspicious. The lock should be held even if ->init_done
is false. Can you explain the difference between init_done
and !init_done?

> +		return -EINVAL;
> +
> +	if (!master->ops->request_mastership)
> +		return -ENOTSUPP;
> +
> +	return master->ops->request_mastership(master);
> +}
> +
> +static int i3c_master_owns_bus(struct i3c_master_controller *master)
> +{
> +	return (master->bus.cur_master == master->this);
> +}
> +
> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master)
> +{
> +	int ret;
> +
> +	if (!i3c_master_owns_bus(master)) {
> +		i3c_bus_normaluse_unlock(&master->bus);
> +		i3c_bus_maintenance_lock(&master->bus);
> +
> +		ret = i3c_master_request_mastership_locked(master);
> +		if (ret) {
> +			i3c_bus_maintenance_unlock(&master->bus);
> +			return ret;
> +		}
> +		i3c_bus_downgrade_maintenance_lock(&master->bus);
> +	}

I think this block deserves a comment: the lock/unlock dance is far
from obvious.

> +
> +	return 0;
> +}
>  static const char * const i3c_bus_mode_strings[] = {
>  	[I3C_BUS_MODE_PURE] = "pure",
>  	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> @@ -636,6 +700,22 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master,
>  	return dev;
>  }
>  
> +static struct i2c_dev_desc *
> +i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master,
> +				      u16 addr, u8 lvr)
> +{
> +	struct i2c_dev_desc *dev;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev->common.master = master;
> +	dev->addr = addr;
> +	dev->lvr = lvr;
> +
> +	return dev;
> +}
>  static void *i3c_ccc_cmd_dest_init(struct i3c_ccc_cmd_dest *dest, u8 addr,
>  				   u16 payloadlen)
>  {
> @@ -705,6 +785,8 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master,
>  	struct i2c_dev_desc *dev;
>  
>  	i3c_bus_for_each_i2cdev(&master->bus, dev) {
> +		if (!dev->boardinfo)
> +			continue;

Blank line here, please.

>  		if (dev->boardinfo->base.addr == addr)
>  			return dev;
>  	}
> @@ -1478,7 +1560,8 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		return;
>  
>  	i3c_bus_for_each_i3cdev(&master->bus, desc) {
> -		if (desc->dev || !desc->info.dyn_addr || desc == master->this)
> +		if (desc->dev || !desc->info.dyn_addr ||
> +		    desc == master->this || !desc->info.pid)

Does the new logic trigger a case where pid == 0? The check looks good
anyway, but I'd rather have it in a separate commit unless you have a
good explanation on why this couldn't happen before this patch.

>  			continue;
>  
>  		desc->dev = kzalloc(sizeof(*desc->dev), GFP_KERNEL);
> @@ -1504,6 +1587,69 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  	}
>  }
>  
> +static struct i2c_dev_boardinfo *
> +i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master,
> +			      u16 addr, u8 lvr)
> +{
> +	struct i2c_dev_boardinfo *i2cboardinfo;
> +
> +	list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
> +		if (i2cboardinfo->base.addr == addr &&
> +		    i2cboardinfo->lvr == lvr)
> +			return i2cboardinfo;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void
> +i3c_master_register_new_i2c_devs(struct i3c_master_controller *master)
> +{
> +	struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master);
> +	struct i2c_dev_desc *i2cdev;
> +
> +	if (!master->init_done)
> +		return;
> +
> +	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> +

You can drop this blank line.

> +		if (i2cdev->dev)
> +			continue;
> +

Add a comment explaining why.

> +		if (!i2cdev->boardinfo)
> +			continue;
> +
> +		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
> +	}
> +}
> +
> +static int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> +					u8 addr)
> +{
> +	struct i3c_ccc_getaccmst *accmst;
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> +	if (!accmst)
> +		return -ENOMEM;
> +
> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> +
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	if (ret)
> +		goto out;
> +
> +	if (dest.payload.len != sizeof(*accmst))
> +		ret = -EIO;
> +
> +out:
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);

Add a blank line here (please run checkpatch --strict, it should
complain about such coding style issues).

>  /**
>   * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
>   * @master: master doing the DAA
> @@ -1548,10 +1694,6 @@ static int i3c_master_set_info(struct i3c_master_controller *master,
>  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
>  		return -EINVAL;
>  
> -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> -	    master->secondary)
> -		return -EINVAL;
> -
>  	if (master->this)
>  		return -EINVAL;
>  
> @@ -1560,7 +1702,8 @@ static int i3c_master_set_info(struct i3c_master_controller *master,
>  		return PTR_ERR(i3cdev);
>  
>  	master->this = i3cdev;
> -	master->bus.cur_master = master->this;
> +	if (!secondary)
> +		master->bus.cur_master = master->this;
>  
>  	ret = i3c_master_attach_i3c_dev(master, i3cdev);
>  	if (ret)
> @@ -1601,37 +1744,7 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
>  	}
>  }
>  

[...]

> +/**
> + * i3c_master_bus_init() - initialize an I3C bus
> + * @master: main master initializing the bus
> + *
> + * This function is following all initialisation steps described in the I3C
> + * specification:
> + *
> + * 1. Attach I2C and statically defined I3C devs to the master so that the
> + *    master can fill its internal device table appropriately
> + *
> + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
> + *    the master controller. That's usually where the bus mode is selected
> + *    (pure bus or mixed fast/slow bus)
> + *
> + * 3. Instruct all devices on the bus to drop their dynamic address. This is
> + *    particularly important when the bus was previously configured by someone
> + *    else (for example the bootloader)
> + *
> + * 4. Disable all slave events.
> + *
> + * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
> + *    devices that have a static address
> + *
> + * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
> + *    remaining I3C devices
> + *
> + * Once this is done, all I3C and I2C devices should be usable.
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +static int i3c_master_bus_init(struct i3c_master_controller *master)
> +{
> +	struct i3c_dev_desc *i3cdev;
> +	int ret;
> +
> +	/*
> +	 * First attach all devices with static definitions provided by the
> +	 * FW.
> +	 */
> +	ret = i3c_master_attach_static_devs(master);
> +	if (ret)
> +		goto err_detach_devs;
>  	/*
>  	 * Now execute the controller specific ->bus_init() routine, which
>  	 * might configure its internal logic to match the bus limitations.
> @@ -1780,45 +1926,76 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  }

The i3c_master_attach_static_devs() split can be done in a separate
commit. Please try to split things as much as possible to ease review.

>  
>  /**
> - * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> - * @master: master used to send frames on the bus
> - * @addr: I3C slave dynamic address assigned to the device
> + * i3c_master_add_i2c_dev_locked() - add an I2C slave to the bus
> + * @master: master used to register I2C device
> + * @addr: I2C device address
> + * @lvr: legacy virtual register value
>   *
> - * This function is instantiating an I3C device object and adding it to the
> - * I3C device list. All device information are automatically retrieved using
> - * standard CCC commands.
> - *
> - * The I3C device object is returned in case the master wants to attach
> - * private data to it using i3c_dev_set_master_data().
> + * This function is instantiating an I2C device object and adding it to the
> + * I2C device list.
>   *
>   * This function must be called with the bus lock held in write mode.
>   *
>   * Return: a 0 in case of success, an negative error code otherwise.
>   */
> -int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> -				  u8 addr)
> +int i3c_master_add_i2c_dev_locked(struct i3c_master_controller *master,
> +				  u16 addr, u8 lvr)
>  {
> -	struct i3c_device_info info = { .dyn_addr = addr };
> -	struct i3c_dev_desc *newdev, *olddev;
> -	u8 old_dyn_addr = addr, expected_dyn_addr;
> -	struct i3c_ibi_setup ibireq = { };
> -	bool enable_ibi = false;
> +	enum i3c_addr_slot_status status;
> +	struct i2c_dev_desc *i2cdev;
>  	int ret;
>  
>  	if (!master)
>  		return -EINVAL;
>  
> -	newdev = i3c_master_alloc_i3c_dev(master, &info);
> -	if (IS_ERR(newdev))
> -		return PTR_ERR(newdev);
> +	status = i3c_bus_get_addr_slot_status(&master->bus,
> +					      addr);
> +	if (status != I3C_ADDR_SLOT_FREE)
> +		return -EBUSY;
>  
> -	ret = i3c_master_attach_i3c_dev(master, newdev);
> -	if (ret)
> +	i3c_bus_set_addr_slot_status(&master->bus, addr,
> +				     I3C_ADDR_SLOT_I2C_DEV);
> +
> +	i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr);
> +
> +	if (IS_ERR(i2cdev)) {
> +		ret = PTR_ERR(i2cdev);
> +		goto err_free_dev;
> +	}
> +
> +	i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr);
> +
> +	ret = i3c_master_attach_i2c_dev(master, i2cdev);
> +
> +	if (ret) {
> +		ret = PTR_ERR(i2cdev);
>  		goto err_free_dev;
> +	}
> +
> +	return 0;
> +
> +err_free_dev:
> +	i3c_bus_set_addr_slot_status(&master->bus, addr,
> +				     I3C_ADDR_SLOT_FREE);
> +	i3c_master_free_i2c_dev(i2cdev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev_locked);

Same here.

> +
> +static int
> +i3c_master_retrieve_info_and_reuse(struct i3c_master_controller *master,
> +				   struct i3c_dev_desc *newdev)
> +{
> +	struct i3c_dev_desc *olddev;
> +	u8 old_dyn_addr = newdev->info.dyn_addr, expected_dyn_addr;
> +	struct i3c_ibi_setup ibireq = { };
> +	bool enable_ibi = false;
> +	int ret;
>  
>  	ret = i3c_master_retrieve_dev_info(newdev);
>  	if (ret)
> -		goto err_detach_dev;
> +		return ret;
>  
>  	olddev = i3c_master_search_i3c_dev_duplicate(newdev);
>  	if (olddev) {
> @@ -1857,7 +2034,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  
>  	ret = i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
>  	if (ret)
> -		goto err_detach_dev;
> +		return ret;
>  
>  	/*
>  	 * Depending on our previous state, the expected dynamic address might
> @@ -1920,6 +2097,50 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  	}
>  
>  	return 0;
> +}
> +
> +/**
> + * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> + * @master: master used to send frames on the bus
> + * @addr: I3C slave dynamic address assigned to the device
> + *
> + * This function is instantiating an I3C device object and adding it to the
> + * I3C device list. All device information are automatically retrieved using
> + * standard CCC commands.
> + *
> + * The I3C device object is returned in case the master wants to attach
> + * private data to it using i3c_dev_set_master_data().
> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> +				  u8 addr)
> +{
> +	struct i3c_device_info info = { .dyn_addr = addr };
> +	struct i3c_dev_desc *newdev;
> +	int ret;
> +
> +	if (!master)
> +		return -EINVAL;
> +
> +	newdev = i3c_master_alloc_i3c_dev(master, &info);
> +	if (IS_ERR(newdev))
> +		return PTR_ERR(newdev);
> +
> +	ret = i3c_master_attach_i3c_dev(master, newdev);
> +	if (ret)
> +		goto err_free_dev;
> +
> +	if (i3c_master_owns_bus(master)) {
> +		ret = i3c_master_retrieve_info_and_reuse(master, newdev);
> +		if (ret)
> +			goto err_detach_dev;
> +	} else
> +		master->want_to_acquire_bus = true;

Please add curly braces.

> +
> +	return 0;
>  
>  err_detach_dev:
>  	if (newdev->dev && newdev->dev->desc)
> @@ -2101,11 +2322,15 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
>  	}
>  
>  	i3c_bus_normaluse_lock(&master->bus);
> +	ret = i3c_master_acquire_bus_ownership(master);
> +	if (ret)
> +		goto err_unlock_bus;
>  	dev = i3c_master_find_i2c_dev_by_addr(master, addr);
>  	if (!dev)
>  		ret = -ENOENT;
>  	else
>  		ret = master->ops->i2c_xfers(dev, xfers, nxfers);
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(&master->bus);
>  
>  	return ret ? ret : nxfers;
> @@ -2144,9 +2369,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master)
>  	 * We silently ignore failures here. The bus should keep working
>  	 * correctly even if one or more i2c devices are not registered.
>  	 */
> -	i3c_bus_for_each_i2cdev(&master->bus, i2cdev)
> +	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> +		if (!i2cdev->boardinfo)
> +			continue;
>  		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
>  
> +	}
>  	return 0;
>  }

[...]

>  /**
>   * i3c_master_init() - initializes all the structures required by I3C master
>   * @master: master used to send frames on the bus
> @@ -2417,9 +2712,6 @@ int i3c_master_init(struct i3c_master_controller *master,
>  	struct i2c_dev_boardinfo *i2cbi;
>  	int ret;
>  
> -	/* We do not support secondary masters yet. */
> -	if (secondary)
> -		return -ENOTSUPP;
>  
>  	ret = i3c_master_check_ops(ops);
>  	if (ret)
> @@ -2432,6 +2724,7 @@ int i3c_master_init(struct i3c_master_controller *master,
>  	master->dev.release = i3c_masterdev_release;
>  	master->ops = ops;
>  	master->secondary = secondary;
> +	master->want_to_acquire_bus = secondary;

Why do we need to set it to true here? Actually, I'm not even sure you
need that variable. Looks like it's only ever set, nothing reads it.

>  	INIT_LIST_HEAD(&master->boardinfo.i2c);
>  	INIT_LIST_HEAD(&master->boardinfo.i3c);
>  
> @@ -2488,6 +2781,92 @@ void i3c_master_cleanup(struct i3c_master_controller *master)
>  EXPORT_SYMBOL_GPL(i3c_master_cleanup);
>  

Overall, I like this version much better than the previous ones. Still
need some refinements though (and more documentation/comments to explain
how that works).


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c