linux-kernel - Re: [PATCH v2 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200812171802.295b08d8@xps13>
Date:   Wed, 12 Aug 2020 17:18:02 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>,
        linux-i3c@...ts.infradead.org, Rob Herring <robh+dt@...nel.org>,
        <devicetree@...r.kernel.org>
Cc:     Nicolas Pitre <nico@...xnic.net>,
        Rajeev Huralikoppi <rajeev.huralikoppi@...vaco.com>,
        Conor Culhane <conor.culhane@...vaco.com>,
        <linux-kernel@...r.kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 3/4] i3c: master: svc: Add Silvaco I3C master driver

Hi Conor, Rajeev,

> +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;
> +	int ret = 0;
> +	u8 *buf;
> +
> +	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;
> +	while (readl(master->regs + SVC_I3C_MSTATUS) & SVC_I3C_MINT_RXPEND) {
> +		mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
> +		count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
> +		readsl(master->regs + SVC_I3C_MRDATAB, buf, count);

After discussing with Boris, I have a question about the design: is
there a way to differentiate, from a software point of view, from the
data coming as "IBI" and "regular data"?

Let's say the master initiates a read.
The moment after, an IBI is triggered.
The hanlde_ibi() helper is called to read the IBI payload.
While the IBI interrupted the read operation, it also interrupted the
master dequeuing process of the bytes already in the FIFO. From a
software perspective, we might end up reading a "regular byte" from the
handle_ibi() helper expecting an "IBI byte".

Is there some kind of double queue feature which I missed? Or perhaps a
way to discriminate the origin of the data?

> +		slot->len += count;
> +		buf += count;
> +	}
> +
> +	i3c_master_queue_ibi(dev, slot);
> +
> +unlock:
> +	spin_unlock(&master->ibi.lock);
> +	svc_i3c_master_emit_stop(master);
> +	svc_i3c_master_flush_fifo(master);
> +
> +	return ret;
> +}
> +
> +static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
> +				   bool mandatory_byte)
> +{
> +	unsigned int ibi_ack_nack;
> +
> +	ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK;
> +	if (mandatory_byte)
> +		ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITH_BYTE;
> +	else
> +		ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE;
> +
> +	writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL);
> +}
> +
> +static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
> +{
> +	writel(SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK |
> +	       SVC_I3C_MCTRL_IBIRESP_NACK,
> +	       master->regs + SVC_I3C_MCTRL);
> +}
> +
> +static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
> +{
> +	struct svc_i3c_master *master = (struct svc_i3c_master *)dev_id;
> +	u32 active = readl(master->regs + SVC_I3C_MINTMASKED);
> +	u32 status = readl(master->regs + SVC_I3C_MSTATUS);
> +	unsigned int ibitype = SVC_I3C_MSTATUS_IBITYPE(status);
> +	unsigned int ibiaddr = SVC_I3C_MSTATUS_IBIADDR(status);
> +	struct i3c_dev_desc *dev;
> +	bool rdata;
> +
> +	if (active & SVC_I3C_MINT_SLVSTART) {
> +		writel(SVC_I3C_MINT_SLVSTART, master->regs + SVC_I3C_MSTATUS);
> +		writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI |
> +		       SVC_I3C_MCTRL_IBIRESP_MANUAL,
> +		       master->regs + SVC_I3C_MCTRL);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (!(active & SVC_I3C_MINT_IBIWON))
> +		return IRQ_NONE;
> +
> +	writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
> +
> +	switch (ibitype) {
> +	case SVC_I3C_MSTATUS_IBITYPE_IBI:
> +		dev = svc_i3c_master_dev_from_addr(master, ibiaddr);
> +		if (WARN_ON(!dev)) {
> +			svc_i3c_master_nack_ibi(master);
> +			break;
> +		}
> +
> +		rdata = dev->info.bcr & I3C_BCR_IBI_PAYLOAD;
> +		svc_i3c_master_ack_ibi(master, rdata);
> +		if (rdata) {
> +			svc_i3c_master_disable_interrupts(master);
> +			return IRQ_WAKE_THREAD;
> +		}
> +
> +		break;
> +	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
> +		svc_i3c_master_nack_ibi(master);
> +		break;
> +	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
> +		svc_i3c_master_ack_ibi(master, false);
> +		queue_work(master->base.wq, &master->hj_work);
> +		break;
> +	default:
> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t svc_i3c_master_threaded_handler(int irq, void *dev_id)
> +{
> +	struct svc_i3c_master *master = (struct svc_i3c_master *)dev_id;
> +	u32 status = readl(master->regs + SVC_I3C_MSTATUS);
> +	unsigned int ibiaddr = SVC_I3C_MSTATUS_IBIADDR(status);
> +	struct i3c_dev_desc *dev;
> +
> +	dev = svc_i3c_master_dev_from_addr(master, ibiaddr);
> +	if (WARN_ON(!dev)) {
> +		svc_i3c_master_emit_stop(master);
> +		svc_i3c_master_flush_fifo(master);
> +		return IRQ_HANDLED;
> +	}
> +
> +	svc_i3c_master_handle_ibi(master, dev);
> +	svc_i3c_master_enable_interrupts(master,
> +					 SVC_I3C_MINT_SLVSTART |
> +					 SVC_I3C_MINT_IBIWON);
> +
> +	return IRQ_HANDLED;
> +}

Thanks,
Miquèl