Re: [PATCH 1/4 net] qca_spi: Fix SPI thread creation

[Stefan Wahren]

Hi Paolo,

Am 23.11.23 um 12:26 schrieb Paolo Abeni:
> On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:
> > The qca_spi driver create/stop the SPI kernel thread in case
> > of netdev_open/close. This is a big issue because it allows
> > userspace to prevent from restarting the SPI thread after
> > ring parameter changes (e.g. signals which stop the thread).
> > This could be done by terminating a script which changes
> > the ring parameter in a loop.
> >
> > So fix this by moving create/stop of the SPI kernel into
> > the init/uninit ops. The open/close ops could be realized just
> > by 'park/unpark' the SPI kernel thread.
> >
> > Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
> > Signed-off-by: Stefan Wahren <wahrenst@xxxxxxx>
> > ---
> >   drivers/net/ethernet/qualcomm/qca_spi.c | 35 ++++++++++++++++---------
> >   1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
> > index bec723028e96..b11a998b2456 100644
> > --- a/drivers/net/ethernet/qualcomm/qca_spi.c
> > +++ b/drivers/net/ethernet/qualcomm/qca_spi.c
> > @@ -580,6 +580,11 @@ qcaspi_spi_thread(void *data)
> >   	netdev_info(qca->net_dev, "SPI thread created\n");
> >   	while (!kthread_should_stop()) {
> >   		set_current_state(TASK_INTERRUPTIBLE);
> > +		if (kthread_should_park()) {
> > +			kthread_parkme();
> > +			continue;
> > +		}
> > +
> >   		if ((qca->intr_req == qca->intr_svc) &&
> >   		    !qca->txr.skb[qca->txr.head])
> >   			schedule();
> > @@ -679,25 +684,17 @@ qcaspi_netdev_open(struct net_device *dev)
> >   	qca->sync = QCASPI_SYNC_UNKNOWN;
> >   	qcafrm_fsm_init_spi(&qca->frm_handle);
> >
> > -	qca->spi_thread = kthread_run((void *)qcaspi_spi_thread,
> > -				      qca, "%s", dev->name);
> > -
> > -	if (IS_ERR(qca->spi_thread)) {
> > -		netdev_err(dev, "%s: unable to start kernel thread.\n",
> > -			   QCASPI_DRV_NAME);
> > -		return PTR_ERR(qca->spi_thread);
> > -	}
> > -
> >   	ret = request_irq(qca->spi_dev->irq, qcaspi_intr_handler, 0,
> >   			  dev->name, qca);
> >   	if (ret) {
> >   		netdev_err(dev, "%s: unable to get IRQ %d (irqval=%d).\n",
> >   			   QCASPI_DRV_NAME, qca->spi_dev->irq, ret);
> > -		kthread_stop(qca->spi_thread);
> >   		return ret;
> >   	}
> >
> >   	/* SPI thread takes care of TX queue */
> > +	kthread_unpark(qca->spi_thread);
> > +	wake_up_process(qca->spi_thread);
> The above looks racy: after 'request_irq()' the interrupt handler can
> raise an irq before the thread being unparked.
yes fixing the whole resource allocation issue requires patch 1 and 2
applied, which should avoid the race. But i didn't want to combine both
patches to keep it applicable for stable. My thought was that 2 smaller
patches are more acceptable than a big one.

Should i squash them?

My concern is about the amount of affected devices. The QCA7000 is used
mostly in EV charging stations and EVs. I don't how many of them use
this driver.

> Additionally I think you can drop the 'if (qca->spi_thread)' in
> qcaspi_intr_handler()
Thanks i will check that for the next version.
> Cheers,
>
> Paolo