Re: [PATCH 3/4] i3c: master: svc: Add Silvaco I3C master driver

[Nicolas Pitre <nico@fluxnic.net>]

On Thu, 9 Jul 2020, Miquel Raynal wrote:

> Add support for Silvaco I3C dual-role IP. The master role is supported
> in SDR mode only. I2C transfers have not been tested but are shared
> because they are so close to the I3C transfers in terms of registers
> configuration.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

I'm worried by constructs like these:

> +static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> +				     struct i3c_dev_desc *dev)
> +{
> +	struct svc_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> +	struct i3c_ibi_slot *slot;
> +	unsigned int count;
> +	u32 mdatactrl;
> +	u8 *buf;
> +	int ret;
> +	u32 reg;
> +
> +	spin_lock(&master->ibi.lock);
> +
> +	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
> +	if (!slot) {
> +		ret = -ENOSPC;
> +		goto unlock;
> +	}
> +
> +	slot->len = 0;
> +	buf = slot->data;
> +
> +	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +				 reg & SVC_I3C_MINT_RXPEND, 0, 1000);

Here you're in atomic context due to the lock, and 
readl_poll_timeout(() is built using usleep() which may ... sleep.

Also, is it actually possible for execution to reach this point if 
SVC_I3C_MINT_RXPEND is not set?

The rest looks reasonable to me.


Nicolas

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