Received-SPF: pass (google.com: domain of linux-kernel+bounces-41916-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <309c899d-551c-41a8-a574-421a796e79cf@gmx.net>
Date: Sun, 28 Jan 2024 20:52:02 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V3 01/14 next] qca_spi: Improve SPI thread creation
To: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
 Eric Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>,
 Lino Sanfilippo <LinoSanfilippo@gmx.de>,
 Florian Fainelli <f.fainelli@gmail.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240124223211.4687-1-wahrenst@gmx.net>
 <20240124223211.4687-2-wahrenst@gmx.net> <20240125183639.585ec73f@kernel.org>
Content-Language: en-US
From: Stefan Wahren <wahrenst@gmx.net>
In-Reply-To: <20240125183639.585ec73f@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:AQKGmC8yLQA=;/V0VdTOAuT6gvYHWNvvRthNQ5kC
 TrF7KSmzhCIlZJDEdwCu7AfEMW8Gi/gcgQAxAq9k0pYAF3JGflNGy8cANF3HrYHi+rNyPC8GL
 WBceOdcmGYRBlyeI5ZIDQhrkDe8+IFMMjLvldcn26l8Yw0vbxIATcxuulI0K4sGcS8VngDIQ2
 3mLG6+4yVxima2E/wBBHxIp5YWulVYVLFHyAWEyuBkZ58StDiDKi8SqZeXAut65a7Biv3XdV8
 GK4+7QnOBCkQUv1myZfi+kk4pw1AFc40ldQU9SskSlxm3CJhELSioXqy5tX1gM9BRlQMSg1//
 z0RYMzDmOLvFbx1lD7iJmEZdJ7wz0ucOdPwW4Pp/c/N/RlwTH2UmyyqjSUzp2bramDjJeV+6G
 ayeXoGNlm+1y/4dmoAYvBqEtOcRhv6+Q7i3D3FkHDczcgEZIOMmidPtuWQXtnkGRh0y+HOKfY
 exRBVDe875FKVYuaXrK9hAYSo1MFjz62GDPoHxA6rdkPGW4oaS/sxXT6Okx/JoxhB7YniwCxH
 2BkI9zDkh9YSQ/1hD1eMUyfoBpyLqt7Avskn8gH3SfjupvU+xDMUi1xL2KHSuoT5+1WZJA46Z
 QRCG3jg290CNiQO6GT5IKc309b/dn+OS8Wf5DM84DNabuOG84Zu6eBOmdSUClIxh8cshnEdjq
 pSogK56flOEfTxsZ37r1GigzzWS0RHfGiUPK8wM0IiD+AR4C6TLIA+eiREDHf3iFRsxjjA3E2
 mv3OyWDso025RtlLN3EhvVGxWE+Xa+/hdN9K+hIxlslxKnC8g85pMs7Mxvlcu5lWerX118SVz
 UmY8AVJljR8EuAGVU+1MJ1VTLiaNdk6COObCbC7/eZhw3sn8p8EF32RIYlm0D9bR/GZ6MP2HO
 xJxxyib6jKNni62vAM4d+K3HPPlk4cP711o3CaUeU4+qAiG3QAFwlMgSDTMjSxYb7SJfaWxGH
 rqEmPLUxItTTi8FEG+2RTjiHRPk=

Hi Jakub,

Am 26.01.24 um 03:36 schrieb Jakub Kicinski:
> On Wed, 24 Jan 2024 23:31:58 +0100 Stefan Wahren wrote:
>> The qca_spi driver create/stop the SPI kernel thread in case
>> of netdev_open/close. This isn't optimal because there is no
>> need for such an expensive operation.
>>
>> So improve this by moving create/stop of the SPI kernel into
>> the init/uninit ops. The open/close ops could just
>> 'park/unpark' the SPI kernel thread.
> What's the concern? I don't think that creating a thread is all
> expensive. And we shouldn't have a thread sitting around when
> the interface isn't use. I mean - if you ask me what's better
> a small chance that the creation will fail at open or having
> a parked and unused thread when device is down - I'd pick
> the former.. But I may well be missing the point.
there is actually another reason for this change which is not mentioned
in the commit message. The pointer spi_thread can have 3 states:
- valid thread
- error pointer
- NULL

So the second motivation was to eliminate the possibility of an error
pointer
by using a local variable. So we only have to care about NULL.

I will change this in V4.
>
>> @@ -825,6 +813,7 @@ static int
>>   qcaspi_netdev_init(struct net_device *dev)
>>   {
>>   	struct qcaspi *qca =3D netdev_priv(dev);
>> +	struct task_struct *thread;
>>
>>   	dev->mtu =3D QCAFRM_MAX_MTU;
>>   	dev->type =3D ARPHRD_ETHER;
>> @@ -848,6 +837,15 @@ qcaspi_netdev_init(struct net_device *dev)
>>   		return -ENOBUFS;
>>   	}
>>
>> +	thread =3D kthread_create(qcaspi_spi_thread, qca, "%s", dev->name);
>> +	if (IS_ERR(thread)) {
>> +		netdev_err(dev, "%s: unable to start kernel thread.\n",
>> +			   QCASPI_DRV_NAME);
>> +		return PTR_ERR(thread);
> I'm 90% sure this leaks resources on failure, too.
Oops

Best regards
>
> Rest of the series LGTM!