[PATCH] rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[PATCH] rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[Emil Renner Berthing]

In commit d3ccc14dfe95 most of the tasklets in this driver was
updated to the new API. However for the rx_work_tasklet only the
type of the callback was changed from
  void _rtl_rx_work(unsigned long data)
to
  void _rtl_rx_work(struct tasklet_struct *t).

The initialization of rx_work_tasklet was still open-coded and the
function pointer just cast into the old type, and hence nothing sets
rx_work_tasklet.use_callback = true and the callback was still called as

  t->func(t->data);

with uninitialized/zero t->data.

Commit 6b8c7574a5f8 changed the casting of _rtl_rx_work a bit and
initialized t->data to a pointer to the tasklet cast to an unsigned
long.

This way calling t->func(t->data) might actually work through all the
casting, but it still doesn't update the code to use the new tasklet
API.

Let's use the new tasklet_setup to initialize rx_work_tasklet properly
and set rx_work_tasklet.use_callback = true so that the callback is
called as

  t->callback(t);

without all the casting.

Fixes: 6b8c7574a5f8 ("rtlwifi: fix build warning")
Fixes: d3ccc14dfe95 ("rtlwifi/rtw88: convert tasklets to use new tasklet_setup() API")
Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
---
 drivers/net/wireless/realtek/rtlwifi/usb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index d62b87f010c9..6c5e242b1bc5 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -310,8 +310,7 @@ static int _rtl_usb_init_rx(struct ieee80211_hw *hw)
 	init_usb_anchor(&rtlusb->rx_cleanup_urbs);
 
 	skb_queue_head_init(&rtlusb->rx_queue);
-	rtlusb->rx_work_tasklet.func = (void(*))_rtl_rx_work;
-	rtlusb->rx_work_tasklet.data = (unsigned long)&rtlusb->rx_work_tasklet;
+	tasklet_setup(&rtlusb->rx_work_tasklet, _rtl_rx_work);
 
 	return 0;
 }
-- 
2.30.0

Re: [PATCH] rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[Willem de Bruijn]

On Wed, Jan 27, 2021 at 5:23 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> In commit d3ccc14dfe95 most of the tasklets in this driver was
> updated to the new API. However for the rx_work_tasklet only the
> type of the callback was changed from
>   void _rtl_rx_work(unsigned long data)
> to
>   void _rtl_rx_work(struct tasklet_struct *t).
>
> The initialization of rx_work_tasklet was still open-coded and the
> function pointer just cast into the old type, and hence nothing sets
> rx_work_tasklet.use_callback = true and the callback was still called as
>
>   t->func(t->data);
>
> with uninitialized/zero t->data.
>
> Commit 6b8c7574a5f8 changed the casting of _rtl_rx_work a bit and
> initialized t->data to a pointer to the tasklet cast to an unsigned
> long.
>
> This way calling t->func(t->data) might actually work through all the
> casting, but it still doesn't update the code to use the new tasklet
> API.
>
> Let's use the new tasklet_setup to initialize rx_work_tasklet properly
> and set rx_work_tasklet.use_callback = true so that the callback is
> called as
>
>   t->callback(t);
>
> without all the casting.
>
> Fixes: 6b8c7574a5f8 ("rtlwifi: fix build warning")
> Fixes: d3ccc14dfe95 ("rtlwifi/rtw88: convert tasklets to use new tasklet_setup() API")
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>

Since the current code works, this could target net-next without Fixes tags.

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH] rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[Kalle Valo]

Willem de Bruijn <willemdebruijn.kernel@gmail.com> writes:

> On Wed, Jan 27, 2021 at 5:23 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
>>
>> In commit d3ccc14dfe95 most of the tasklets in this driver was
>> updated to the new API. However for the rx_work_tasklet only the
>> type of the callback was changed from
>>   void _rtl_rx_work(unsigned long data)
>> to
>>   void _rtl_rx_work(struct tasklet_struct *t).
>>
>> The initialization of rx_work_tasklet was still open-coded and the
>> function pointer just cast into the old type, and hence nothing sets
>> rx_work_tasklet.use_callback = true and the callback was still called as
>>
>>   t->func(t->data);
>>
>> with uninitialized/zero t->data.
>>
>> Commit 6b8c7574a5f8 changed the casting of _rtl_rx_work a bit and
>> initialized t->data to a pointer to the tasklet cast to an unsigned
>> long.
>>
>> This way calling t->func(t->data) might actually work through all the
>> casting, but it still doesn't update the code to use the new tasklet
>> API.
>>
>> Let's use the new tasklet_setup to initialize rx_work_tasklet properly
>> and set rx_work_tasklet.use_callback = true so that the callback is
>> called as
>>
>>   t->callback(t);
>>
>> without all the casting.
>>
>> Fixes: 6b8c7574a5f8 ("rtlwifi: fix build warning")
>> Fixes: d3ccc14dfe95 ("rtlwifi/rtw88: convert tasklets to use new
>> tasklet_setup() API")
>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>
> Since the current code works, this could target net-next

This should go to wireless-drivers-next, not net-next.

> without Fixes tags.

Correct, no need for Fixes tag as there's no bug to fix. This is only
cleanup AFAICS.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[Emil Renner Berthing]

On Wed, 27 Jan 2021 at 16:20, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Willem de Bruijn <willemdebruijn.kernel@gmail.com> writes:
>
> > On Wed, Jan 27, 2021 at 5:23 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
> >>
> >> In commit d3ccc14dfe95 most of the tasklets in this driver was
> >> updated to the new API. However for the rx_work_tasklet only the
> >> type of the callback was changed from
> >>   void _rtl_rx_work(unsigned long data)
> >> to
> >>   void _rtl_rx_work(struct tasklet_struct *t).
> >>
> >> The initialization of rx_work_tasklet was still open-coded and the
> >> function pointer just cast into the old type, and hence nothing sets
> >> rx_work_tasklet.use_callback = true and the callback was still called as
> >>
> >>   t->func(t->data);
> >>
> >> with uninitialized/zero t->data.
> >>
> >> Commit 6b8c7574a5f8 changed the casting of _rtl_rx_work a bit and
> >> initialized t->data to a pointer to the tasklet cast to an unsigned
> >> long.
> >>
> >> This way calling t->func(t->data) might actually work through all the
> >> casting, but it still doesn't update the code to use the new tasklet
> >> API.
> >>
> >> Let's use the new tasklet_setup to initialize rx_work_tasklet properly
> >> and set rx_work_tasklet.use_callback = true so that the callback is
> >> called as
> >>
> >>   t->callback(t);
> >>
> >> without all the casting.
> >>
> >> Fixes: 6b8c7574a5f8 ("rtlwifi: fix build warning")
> >> Fixes: d3ccc14dfe95 ("rtlwifi/rtw88: convert tasklets to use new
> >> tasklet_setup() API")
> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> >
> > Since the current code works, this could target net-next
>
> This should go to wireless-drivers-next, not net-next.
>
> > without Fixes tags.
>
> Correct, no need for Fixes tag as there's no bug to fix. This is only
> cleanup AFAICS.

I can definitely see how you can reasonably disagree, but I would not
be comfortable having code that only works because the calling
conventions of all relevant architectures happen to put the first
unsigned long argument and the first pointer argument in the same
register.
If you want to be conservative I'd much rather revert all the changes
around rx_work_tasklet including the new type of the _rtl_rx_work
callback, so we don't have to rely on calling conventions matching up.

In any case: as long as this fix eventually ends up upstream I'm fine.

/Emil

Re: [PATCH] rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[Kalle Valo]

Emil Renner Berthing <kernel@esmil.dk> writes:

> On Wed, 27 Jan 2021 at 16:20, Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Willem de Bruijn <willemdebruijn.kernel@gmail.com> writes:
>>
>> > On Wed, Jan 27, 2021 at 5:23 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
>> >>
>> >> In commit d3ccc14dfe95 most of the tasklets in this driver was
>> >> updated to the new API. However for the rx_work_tasklet only the
>> >> type of the callback was changed from
>> >>   void _rtl_rx_work(unsigned long data)
>> >> to
>> >>   void _rtl_rx_work(struct tasklet_struct *t).
>> >>
>> >> The initialization of rx_work_tasklet was still open-coded and the
>> >> function pointer just cast into the old type, and hence nothing sets
>> >> rx_work_tasklet.use_callback = true and the callback was still called as
>> >>
>> >>   t->func(t->data);
>> >>
>> >> with uninitialized/zero t->data.
>> >>
>> >> Commit 6b8c7574a5f8 changed the casting of _rtl_rx_work a bit and
>> >> initialized t->data to a pointer to the tasklet cast to an unsigned
>> >> long.
>> >>
>> >> This way calling t->func(t->data) might actually work through all the
>> >> casting, but it still doesn't update the code to use the new tasklet
>> >> API.
>> >>
>> >> Let's use the new tasklet_setup to initialize rx_work_tasklet properly
>> >> and set rx_work_tasklet.use_callback = true so that the callback is
>> >> called as
>> >>
>> >>   t->callback(t);
>> >>
>> >> without all the casting.
>> >>
>> >> Fixes: 6b8c7574a5f8 ("rtlwifi: fix build warning")
>> >> Fixes: d3ccc14dfe95 ("rtlwifi/rtw88: convert tasklets to use new
>> >> tasklet_setup() API")
>> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>> >
>> > Since the current code works, this could target net-next
>>
>> This should go to wireless-drivers-next, not net-next.
>>
>> > without Fixes tags.
>>
>> Correct, no need for Fixes tag as there's no bug to fix. This is only
>> cleanup AFAICS.

Forgot to mention that I can remove the Fixes tags during commit, so no
need to resend just because of those.

> I can definitely see how you can reasonably disagree, but I would not
> be comfortable having code that only works because the calling
> conventions of all relevant architectures happen to put the first
> unsigned long argument and the first pointer argument in the same
> register.

If there's a bug this patch fixes please explain it clearly in the
commit log. But as I read it (though I admit very quickly) I understood
this is just cleanup.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[Emil Renner Berthing]

On Wed, 27 Jan 2021 at 16:33, Kalle Valo <kvalo@codeaurora.org> wrote:
> ...
> Forgot to mention that I can remove the Fixes tags during commit, so no
> need to resend just because of those.

Cool, thanks.

> > I can definitely see how you can reasonably disagree, but I would not
> > be comfortable having code that only works because the calling
> > conventions of all relevant architectures happen to put the first
> > unsigned long argument and the first pointer argument in the same
> > register.
>
> If there's a bug this patch fixes please explain it clearly in the
> commit log. But as I read it (though I admit very quickly) I understood
> this is just cleanup.

Sorry, I'll try again.

So the tasklet_struct looks like this:
struct tasklet_struct {
  ..
  bool use_callback;
  union {
    void (*func)(unsigned long);
    void (*callback)(struct tasklet_struct *);
  };
  unsigned long data;
};

..and the use_callback flag is used like this:
if (t->use_callback)
  t->callback(t);
else
  t->func(t->data);

Now commit d3ccc14dfe95 changed the _rtl_rx_work to be of the new
callback, not func, type. But it didn't actually set the use_callback
flag, and just typecast the _rtl_rx_work function pointer and assigned
it to the func member. So _rtl_rx_work is still called as
t->func(t->data) even though it was rewritten to be called as
t->callback(t).
Now 6b8c7574a5f8 set t->data = (unsigned long)t, so calling
t->func(t->data) will probably work on most architectures because:

a) "unsigned long" and "struct tasklet_struct *" has the same width on
all Linux-capable architectures and
b) calling t->func(t->data) will put the value from t->data into the
same register as the function
    void _rtl_rx_work(struct tasklet_struct *t)
  expects to find the pointer t in the C calling conventions used by
all relevant architectures.

I guess it's debatable weather this is a bug or just ugly code.

/Emil

Re: rtlwifi: use tasklet_setup to initialize rx_work_tasklet

[Kalle Valo]

Emil Renner Berthing <kernel@esmil.dk> wrote:

> In commit d3ccc14dfe95 most of the tasklets in this driver was
> updated to the new API. However for the rx_work_tasklet only the
> type of the callback was changed from
>   void _rtl_rx_work(unsigned long data)
> to
>   void _rtl_rx_work(struct tasklet_struct *t).
> 
> The initialization of rx_work_tasklet was still open-coded and the
> function pointer just cast into the old type, and hence nothing sets
> rx_work_tasklet.use_callback = true and the callback was still called as
> 
>   t->func(t->data);
> 
> with uninitialized/zero t->data.
> 
> Commit 6b8c7574a5f8 changed the casting of _rtl_rx_work a bit and
> initialized t->data to a pointer to the tasklet cast to an unsigned
> long.
> 
> This way calling t->func(t->data) might actually work through all the
> casting, but it still doesn't update the code to use the new tasklet
> API.
> 
> Let's use the new tasklet_setup to initialize rx_work_tasklet properly
> and set rx_work_tasklet.use_callback = true so that the callback is
> called as
> 
>   t->callback(t);
> 
> without all the casting.
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> Acked-by: Willem de Bruijn <willemb@google.com>

Patch applied to wireless-drivers-next.git, thanks.

ca04217add8e rtlwifi: use tasklet_setup to initialize rx_work_tasklet

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210126171550.3066-1-kernel@esmil.dk/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches