linux-kernel - RE: [PATCH net-next v9 2/2] net: ethernet: Add driver for Sunplus SP7021

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: <ff2077684c4c45fca929a8f61447242b@sphcmbx02.sunplus.com.tw>
Date:   Thu, 28 Apr 2022 11:32:56 +0000
From:   Wells Lu 呂芳騰 <wells.lu@...plus.com>
To:     Francois Romieu <romieu@...zoreil.com>,
        Wells Lu <wellslutw@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
        "roopa@...dia.com" <roopa@...dia.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "edumazet@...gle.com" <edumazet@...gle.com>
Subject: RE: [PATCH net-next v9 2/2] net: ethernet: Add driver for Sunplus
 SP7021

Hi Francois,


> [...]
> > +int spl2sw_rx_poll(struct napi_struct *napi, int budget) {
> [...]
> > +	wmb();	/* make sure settings are effective. */
> > +	mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> > +	mask &= ~MAC_INT_RX;
> > +	writel(mask, comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> > +
> > +	napi_complete(napi);
> > +	return 0;
> > +}
> > +
> > +int spl2sw_tx_poll(struct napi_struct *napi, int budget) {
> [...]
> > +	wmb();			/* make sure settings are effective. */
> > +	mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> > +	mask &= ~MAC_INT_TX;
> > +	writel(mask, comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> > +
> > +	napi_complete(napi);
> > +	return 0;
> > +}
> > +
> > +irqreturn_t spl2sw_ethernet_interrupt(int irq, void *dev_id) {
> [...]
> > +	if (status & MAC_INT_RX) {
> > +		/* Disable RX interrupts. */
> > +		mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> > +		mask |= MAC_INT_RX;
> > +		writel(mask, comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> [...]
> > +		napi_schedule(&comm->rx_napi);
> > +	}
> > +
> > +	if (status & MAC_INT_TX) {
> > +		/* Disable TX interrupts. */
> > +		mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> > +		mask |= MAC_INT_TX;
> > +		writel(mask, comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
> > +
> > +		if (unlikely(status & MAC_INT_TX_DES_ERR)) {
> [...]
> > +		} else {
> > +			napi_schedule(&comm->tx_napi);
> > +		}
> > +	}
> 
> The readl/writel sequence in rx_poll (or tx_poll) races with the irq handler performing
> MAC_INT_TX (or MAC_INT_RX) work. If the readl returns the same value to both callers,
> one of the writel will be overwritten.
> 
> --
> Ueimor

I will add disable_irq() and enable_irq() for spl2sw_rx_poll() and spl2sw_tx_poll() as shown below:

spl2sw_rx_poll():

	wmb();	/* make sure settings are effective. */
	disable_irq(comm->irq);
	mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
	mask &= ~MAC_INT_RX;
	writel(mask, comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
	enable_irq(comm->irq);

spl2sw_tx_poll():

	wmb();			/* make sure settings are effective. */
	disable_irq(comm->irq);
	mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
	mask &= ~MAC_INT_TX;
	writel(mask, comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
	enable_irq(comm->irq);


Is the modification ok?


Thank you for your review.

Best regards,
Wells