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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 3 Aug 2020 16:20:11 -0400 (EDT)
From:   Nicolas Pitre <nico@...xnic.net>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
cc:     Boris Brezillon <boris.brezillon@...labora.com>,
        linux-i3c@...ts.infradead.org,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Conor Culhane <conor.culhane@...vaco.com>,
        Rajeev Huralikoppi <rajeev.huralikoppi@...vaco.com>,
        npitre@...libre.com
Subject: Re: [PATCH 3/4] i3c: master: svc: Add Silvaco I3C master driver

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@...tlin.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