The zero day bot reported some sparse complaints in pata_falcon.c. E.g.
drivers/ata/pata_falcon.c:58:41: warning: cast removes address space '__iomem' of expression
drivers/ata/pata_falcon.c:58:41: warning: incorrect type in argument 1 (different address spaces)
drivers/ata/pata_falcon.c:58:41: expected unsigned short volatile [noderef] [usertype] __iomem *port
drivers/ata/pata_falcon.c:58:41: got unsigned short [usertype] *
The same thing shows up in 8 places, all told. Avoid this by use of
__iomem type casts.
Cc: Michael Schmitz <[email protected]>
Cc: Bartlomiej Zolnierkiewicz <[email protected]>
Cc: Jens Axboe <[email protected]>
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/ata/pata_falcon.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
index 121635aa8c00..e2a88edd9dbf 100644
--- a/drivers/ata/pata_falcon.c
+++ b/drivers/ata/pata_falcon.c
@@ -55,14 +55,14 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
/* Transfer multiple of 2 bytes */
if (rw == READ) {
if (swap)
- raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
+ raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
else
- raw_insw((u16 *)data_addr, (u16 *)buf, words);
+ raw_insw((u16 __iomem *)data_addr, (u16 *)buf, words);
} else {
if (swap)
- raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
+ raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
else
- raw_outsw((u16 *)data_addr, (u16 *)buf, words);
+ raw_outsw((u16 __iomem *)data_addr, (u16 *)buf, words);
}
/* Transfer trailing byte, if any. */
@@ -74,16 +74,16 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
if (rw == READ) {
if (swap)
- raw_insw_swapw((u16 *)data_addr, (u16 *)pad, 1);
+ raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)pad, 1);
else
- raw_insw((u16 *)data_addr, (u16 *)pad, 1);
+ raw_insw((u16 __iomem *)data_addr, (u16 *)pad, 1);
*buf = pad[0];
} else {
pad[0] = *buf;
if (swap)
- raw_outsw_swapw((u16 *)data_addr, (u16 *)pad, 1);
+ raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)pad, 1);
else
- raw_outsw((u16 *)data_addr, (u16 *)pad, 1);
+ raw_outsw((u16 __iomem *)data_addr, (u16 *)pad, 1);
}
words++;
}
--
2.26.3
Hi Finn,
thanks for your patch!
On 24/11/21 17:25, Finn Thain wrote:
> The zero day bot reported some sparse complaints in pata_falcon.c. E.g.
>
> drivers/ata/pata_falcon.c:58:41: warning: cast removes address space '__iomem' of expression
> drivers/ata/pata_falcon.c:58:41: warning: incorrect type in argument 1 (different address spaces)
> drivers/ata/pata_falcon.c:58:41: expected unsigned short volatile [noderef] [usertype] __iomem *port
> drivers/ata/pata_falcon.c:58:41: got unsigned short [usertype] *
>
> The same thing shows up in 8 places, all told. Avoid this by use of
> __iomem type casts.
Seeing as data_addr was explicitly typed as __iomem, your fix is clearly
correct. Bit embarrassing to have missed that (I remember adding __iomem
annotations elsewhere at some stage).
If you think there's any need to test this change, say so.
Reviewed-by: Michael Schmitz <[email protected]>
>
> Cc: Michael Schmitz <[email protected]>
> Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
> ---
> drivers/ata/pata_falcon.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
> index 121635aa8c00..e2a88edd9dbf 100644
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
> @@ -55,14 +55,14 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
> /* Transfer multiple of 2 bytes */
> if (rw == READ) {
> if (swap)
> - raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
> + raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> else
> - raw_insw((u16 *)data_addr, (u16 *)buf, words);
> + raw_insw((u16 __iomem *)data_addr, (u16 *)buf, words);
> } else {
> if (swap)
> - raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
> + raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> else
> - raw_outsw((u16 *)data_addr, (u16 *)buf, words);
> + raw_outsw((u16 __iomem *)data_addr, (u16 *)buf, words);
> }
>
> /* Transfer trailing byte, if any. */
> @@ -74,16 +74,16 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>
> if (rw == READ) {
> if (swap)
> - raw_insw_swapw((u16 *)data_addr, (u16 *)pad, 1);
> + raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)pad, 1);
> else
> - raw_insw((u16 *)data_addr, (u16 *)pad, 1);
> + raw_insw((u16 __iomem *)data_addr, (u16 *)pad, 1);
> *buf = pad[0];
> } else {
> pad[0] = *buf;
> if (swap)
> - raw_outsw_swapw((u16 *)data_addr, (u16 *)pad, 1);
> + raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)pad, 1);
> else
> - raw_outsw((u16 *)data_addr, (u16 *)pad, 1);
> + raw_outsw((u16 __iomem *)data_addr, (u16 *)pad, 1);
> }
> words++;
> }
>
Hi Finn,
On Wed, Nov 24, 2021 at 8:36 AM Finn Thain <[email protected]> wrote:
> The zero day bot reported some sparse complaints in pata_falcon.c. E.g.
>
> drivers/ata/pata_falcon.c:58:41: warning: cast removes address space '__iomem' of expression
> drivers/ata/pata_falcon.c:58:41: warning: incorrect type in argument 1 (different address spaces)
> drivers/ata/pata_falcon.c:58:41: expected unsigned short volatile [noderef] [usertype] __iomem *port
> drivers/ata/pata_falcon.c:58:41: got unsigned short [usertype] *
>
> The same thing shows up in 8 places, all told. Avoid this by use of
> __iomem type casts.
>
> Cc: Michael Schmitz <[email protected]>
> Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
Thanks for your patch!
> --- a/drivers/ata/pata_falcon.c
> +++ b/drivers/ata/pata_falcon.c
> @@ -55,14 +55,14 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
> /* Transfer multiple of 2 bytes */
> if (rw == READ) {
> if (swap)
> - raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
> + raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> else
> - raw_insw((u16 *)data_addr, (u16 *)buf, words);
> + raw_insw((u16 __iomem *)data_addr, (u16 *)buf, words);
> } else {
> if (swap)
> - raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
> + raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> else
> - raw_outsw((u16 *)data_addr, (u16 *)buf, words);
> + raw_outsw((u16 __iomem *)data_addr, (u16 *)buf, words);
Can't you just drop the casts? data_addr is an __iomem void *.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Wed, Nov 24, 2021 at 8:51 PM Geert Uytterhoeven <[email protected]> wrote:
> > --- a/drivers/ata/pata_falcon.c
> > +++ b/drivers/ata/pata_falcon.c
> > @@ -55,14 +55,14 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
> > /* Transfer multiple of 2 bytes */
> > if (rw == READ) {
> > if (swap)
> > - raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > else
> > - raw_insw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_insw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > } else {
> > if (swap)
> > - raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > else
> > - raw_outsw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_outsw((u16 __iomem *)data_addr, (u16 *)buf, words);
>
> Can't you just drop the casts? data_addr is an __iomem void *.
It's not u16 though, and the raw_ IO functions require that. But we
could cast data_addr as __iomem u16 * (compile tested).
Cheers,
Michael
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
On Wed, 24 Nov 2021, Geert Uytterhoeven wrote:
> Thanks for your patch!
>
> > --- a/drivers/ata/pata_falcon.c
> > +++ b/drivers/ata/pata_falcon.c
> > @@ -55,14 +55,14 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
> > /* Transfer multiple of 2 bytes */
> > if (rw == READ) {
> > if (swap)
> > - raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > else
> > - raw_insw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_insw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > } else {
> > if (swap)
> > - raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > else
> > - raw_outsw((u16 *)data_addr, (u16 *)buf, words);
> > + raw_outsw((u16 __iomem *)data_addr, (u16 *)buf, words);
>
> Can't you just drop the casts? data_addr is an __iomem void *.
>
Yes, that works here (i.e. removing the data_addr casts and not the buf
casts). But is it prudent?
Given the implementation of raw_in/out is subject to change, it seems like
the original casts were defensive programming.
Here's an example of a recent regression that was fixed by casting a macro
argument to a specific width:
https://lore.kernel.org/linuxppc-dev/[email protected]/
https://lore.kernel.org/linuxppc-dev/08bbe7240b384016e0b2912ecf3bf5e2d25ef2c6.1636501628.git.fthain@linux-m68k.org/
BTW, that bug eventually got fixed using a different patch.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/powerpc/kernel/signal.h?id=5499802b2284331788a440585869590f1bd63f7f
Note that __get_user_sigset() still lacks width casts here, so it remains
non-portable.
On Wed, 24 Nov 2021, Michael Schmitz wrote:
> On 24/11/21 17:25, Finn Thain wrote:
> > The zero day bot reported some sparse complaints in pata_falcon.c. E.g.
> >
> > drivers/ata/pata_falcon.c:58:41: warning: cast removes address space
> > '__iomem' of expression
> > drivers/ata/pata_falcon.c:58:41: warning: incorrect type in argument 1
> > (different address spaces)
> > drivers/ata/pata_falcon.c:58:41: expected unsigned short volatile
> > [noderef] [usertype] __iomem *port
> > drivers/ata/pata_falcon.c:58:41: got unsigned short [usertype] *
> >
> > The same thing shows up in 8 places, all told. Avoid this by use of
> > __iomem type casts.
>
> Seeing as data_addr was explicitly typed as __iomem, your fix is clearly
> correct. Bit embarrassing to have missed that (I remember adding __iomem
> annotations elsewhere at some stage).
>
> If you think there's any need to test this change, say so.
>
There's no change in pata_falcon.o.
> Reviewed-by: Michael Schmitz <[email protected]>
>
Thanks.
Hi Michael,
On Wed, Nov 24, 2021 at 9:50 PM Michael Schmitz <[email protected]> wrote:
> On Wed, Nov 24, 2021 at 8:51 PM Geert Uytterhoeven <[email protected]> wrote:
> > > --- a/drivers/ata/pata_falcon.c
> > > +++ b/drivers/ata/pata_falcon.c
> > > @@ -55,14 +55,14 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
> > > /* Transfer multiple of 2 bytes */
> > > if (rw == READ) {
> > > if (swap)
> > > - raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > > else
> > > - raw_insw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_insw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > > } else {
> > > if (swap)
> > > - raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > > else
> > > - raw_outsw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_outsw((u16 __iomem *)data_addr, (u16 *)buf, words);
> >
> > Can't you just drop the casts? data_addr is an __iomem void *.
>
> It's not u16 though, and the raw_ IO functions require that. But we
> could cast data_addr as __iomem u16 * (compile tested).
The raw_ IO functions do not require that: you can pass a void *
to a function that expects a different pointer type.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Finn,
On Thu, Nov 25, 2021 at 2:06 AM Finn Thain <[email protected]> wrote:
> On Wed, 24 Nov 2021, Geert Uytterhoeven wrote:
> > > --- a/drivers/ata/pata_falcon.c
> > > +++ b/drivers/ata/pata_falcon.c
> > > @@ -55,14 +55,14 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
> > > /* Transfer multiple of 2 bytes */
> > > if (rw == READ) {
> > > if (swap)
> > > - raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_insw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > > else
> > > - raw_insw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_insw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > > } else {
> > > if (swap)
> > > - raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_outsw_swapw((u16 __iomem *)data_addr, (u16 *)buf, words);
> > > else
> > > - raw_outsw((u16 *)data_addr, (u16 *)buf, words);
> > > + raw_outsw((u16 __iomem *)data_addr, (u16 *)buf, words);
> >
> > Can't you just drop the casts? data_addr is an __iomem void *.
>
> Yes, that works here (i.e. removing the data_addr casts and not the buf
> casts). But is it prudent?
>
> Given the implementation of raw_in/out is subject to change, it seems like
> the original casts were defensive programming.
>
> Here's an example of a recent regression that was fixed by casting a macro
> argument to a specific width:
>
> https://lore.kernel.org/linuxppc-dev/[email protected]/
> https://lore.kernel.org/linuxppc-dev/08bbe7240b384016e0b2912ecf3bf5e2d25ef2c6.1636501628.git.fthain@linux-m68k.org/
Yeah, you do have to be careful with macros that derive a size from
the type of the passed data. The *{in,out}sw() functions do not suffer
from that: they are defined to operate on a 16-bit I/O register.
It is very unlikely these semantics will ever change.
Here I'm more worried about the other danger: keeping casts will
silence any warning that may be introduced in a future change to the
driver code.
BTW, insw() and readsw() in asm-generic take void *.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 25 Nov 2021, Geert Uytterhoeven wrote:
>
> Yeah, you do have to be careful with macros that derive a size from the
> type of the passed data. The *{in,out}sw() functions do not suffer from
> that: they are defined to operate on a 16-bit I/O register. It is very
> unlikely these semantics will ever change.
>
> Here I'm more worried about the other danger: keeping casts will silence
> any warning that may be introduced in a future change to the driver
> code.
>
Fair enough. I'll send v2.