| Message ID | 44e0213a681f3c8ee4c6ab2ef9d61ce3ac00e368.1637727935.git.fthain@linux-m68k.org |
|---|---|
| State | New |
| Headers | show |
| Series | pata_falcon: Add missing __iomem annotations | expand |
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 <schmitzmic@gmail.com> > > Cc: Michael Schmitz <schmitzmic@gmail.com> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Cc: Jens Axboe <axboe@kernel.dk> > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Finn Thain <fthain@linux-m68k.org> > --- > 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 <fthain@linux-m68k.org> 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 <schmitzmic@gmail.com> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Cc: Jens Axboe <axboe@kernel.dk> > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Finn Thain <fthain@linux-m68k.org> 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 -- geert@linux-m68k.org 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 <geert@linux-m68k.org> 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 -- geert@linux-m68k.org > > 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/79ae1f49-f6b1-e9ad-977d-0cc7e553c7b9@csgroup.eu/ 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 <schmitzmic@gmail.com> > Thanks.
Hi Michael, On Wed, Nov 24, 2021 at 9:50 PM Michael Schmitz <schmitzmic@gmail.com> wrote: > On Wed, Nov 24, 2021 at 8:51 PM Geert Uytterhoeven <geert@linux-m68k.org> 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 -- geert@linux-m68k.org 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 <fthain@linux-m68k.org> 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/79ae1f49-f6b1-e9ad-977d-0cc7e553c7b9@csgroup.eu/ > 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 -- geert@linux-m68k.org 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.
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++; }
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 <schmitzmic@gmail.com> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Cc: Jens Axboe <axboe@kernel.dk> Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Finn Thain <fthain@linux-m68k.org> --- drivers/ata/pata_falcon.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)