[PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors

[PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors

[Nathan Chancellor]

Clang warns:

drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
[-Werror,-Wempty-body]
        if (Read_hfc(cs, HFCPCI_INT_S1));
                                        ^
drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
separate line to silence this warning

Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting
the return of Read_hfc to void, instead of using an empty if statement.

While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc
should be using a standard access method in hfc_pci.h. Use the one found
in drivers/isdn/hardware/mISDN/hfc_pci.h.

Link: https://github.com/ClangBuiltLinux/linux/issues/66
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/isdn/hisax/hfc_pci.c | 6 +++---
 drivers/isdn/hisax/hfc_pci.h | 4 ++--
 drivers/isdn/hisax/hfc_sx.c  | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index 8e5b03161b2f..a63b9155b697 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
 	Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
 
 	/* Clear already pending ints */
-	if (Read_hfc(cs, HFCPCI_INT_S1));
+	(void) Read_hfc(cs, HFCPCI_INT_S1);
 
 	Write_hfc(cs, HFCPCI_STATES, HFCPCI_LOAD_STATE | 2);	/* HFC ST 2 */
 	udelay(10);
@@ -158,7 +158,7 @@ reset_hfcpci(struct IsdnCardState *cs)
 	/* Finally enable IRQ output */
 	cs->hw.hfcpci.int_m2 = HFCPCI_IRQ_ENABLE;
 	Write_hfc(cs, HFCPCI_INT_M2, cs->hw.hfcpci.int_m2);
-	if (Read_hfc(cs, HFCPCI_INT_S1));
+	(void) Read_hfc(cs, HFCPCI_INT_S1);
 }
 
 /***************************************************/
@@ -1537,7 +1537,7 @@ hfcpci_bh(struct work_struct *work)
 					cs->hw.hfcpci.int_m1 &= ~HFCPCI_INTS_TIMER;
 					Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
 					/* Clear already pending ints */
-					if (Read_hfc(cs, HFCPCI_INT_S1));
+					(void) Read_hfc(cs, HFCPCI_INT_S1);
 					Write_hfc(cs, HFCPCI_STATES, 4 | HFCPCI_LOAD_STATE);
 					udelay(10);
 					Write_hfc(cs, HFCPCI_STATES, 4);
diff --git a/drivers/isdn/hisax/hfc_pci.h b/drivers/isdn/hisax/hfc_pci.h
index 4e58700a3e61..4c3b3ba35726 100644
--- a/drivers/isdn/hisax/hfc_pci.h
+++ b/drivers/isdn/hisax/hfc_pci.h
@@ -228,8 +228,8 @@ typedef union {
 } fifo_area;
 
 
-#define Write_hfc(a, b, c) (*(((u_char *)a->hw.hfcpci.pci_io) + b) = c)
-#define Read_hfc(a, b) (*(((u_char *)a->hw.hfcpci.pci_io) + b))
+#define Write_hfc(a, b, c) (writeb(c, (a->hw.hfcpci.pci_io) + b))
+#define Read_hfc(a, b) (readb((a->hw.hfcpci.pci_io) + b))
 
 extern void main_irq_hcpci(struct BCState *bcs);
 extern void releasehfcpci(struct IsdnCardState *cs);
diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c
index 4d3b4b2f2612..c4f3f37adfc8 100644
--- a/drivers/isdn/hisax/hfc_sx.c
+++ b/drivers/isdn/hisax/hfc_sx.c
@@ -381,7 +381,7 @@ reset_hfcsx(struct IsdnCardState *cs)
 	Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
 
 	/* Clear already pending ints */
-	if (Read_hfc(cs, HFCSX_INT_S1));
+	(void) Read_hfc(cs, HFCSX_INT_S1);
 
 	Write_hfc(cs, HFCSX_STATES, HFCSX_LOAD_STATE | 2);	/* HFC ST 2 */
 	udelay(10);
@@ -411,7 +411,7 @@ reset_hfcsx(struct IsdnCardState *cs)
 	/* Finally enable IRQ output */
 	cs->hw.hfcsx.int_m2 = HFCSX_IRQ_ENABLE;
 	Write_hfc(cs, HFCSX_INT_M2, cs->hw.hfcsx.int_m2);
-	if (Read_hfc(cs, HFCSX_INT_S2));
+	(void) Read_hfc(cs, HFCSX_INT_S2);
 }
 
 /***************************************************/
@@ -1288,7 +1288,7 @@ hfcsx_bh(struct work_struct *work)
 					cs->hw.hfcsx.int_m1 &= ~HFCSX_INTS_TIMER;
 					Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
 					/* Clear already pending ints */
-					if (Read_hfc(cs, HFCSX_INT_S1));
+					(void) Read_hfc(cs, HFCSX_INT_S1);
 
 					Write_hfc(cs, HFCSX_STATES, 4 | HFCSX_LOAD_STATE);
 					udelay(10);
-- 
2.19.1

Re: [PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors

[Masahiro Yamada]

Hi Nathan,

On Thu, Oct 18, 2018 at 3:09 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns:
>
> drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
> [-Werror,-Wempty-body]
>         if (Read_hfc(cs, HFCPCI_INT_S1));
>                                         ^
> drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
> separate line to silence this warning
>
> Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting
> the return of Read_hfc to void, instead of using an empty if statement.
>
> While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc
> should be using a standard access method in hfc_pci.h. Use the one found
> in drivers/isdn/hardware/mISDN/hfc_pci.h.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/66
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/isdn/hisax/hfc_pci.c | 6 +++---
>  drivers/isdn/hisax/hfc_pci.h | 4 ++--
>  drivers/isdn/hisax/hfc_sx.c  | 6 +++---
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
> index 8e5b03161b2f..a63b9155b697 100644
> --- a/drivers/isdn/hisax/hfc_pci.c
> +++ b/drivers/isdn/hisax/hfc_pci.c
> @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
>         Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
>
>         /* Clear already pending ints */
> -       if (Read_hfc(cs, HFCPCI_INT_S1));
> +       (void) Read_hfc(cs, HFCPCI_INT_S1);



Why is this '(void)' necessary?

I see no warning without it.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors

[Nathan Chancellor]

On Thu, Oct 18, 2018 at 12:23:37PM +0900, Masahiro Yamada wrote:
> Hi Nathan,
> 
> On Thu, Oct 18, 2018 at 3:09 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns:
> >
> > drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
> > [-Werror,-Wempty-body]
> >         if (Read_hfc(cs, HFCPCI_INT_S1));
> >                                         ^
> > drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
> > separate line to silence this warning
> >
> > Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting
> > the return of Read_hfc to void, instead of using an empty if statement.
> >
> > While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc
> > should be using a standard access method in hfc_pci.h. Use the one found
> > in drivers/isdn/hardware/mISDN/hfc_pci.h.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/66
> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/isdn/hisax/hfc_pci.c | 6 +++---
> >  drivers/isdn/hisax/hfc_pci.h | 4 ++--
> >  drivers/isdn/hisax/hfc_sx.c  | 6 +++---
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
> > index 8e5b03161b2f..a63b9155b697 100644
> > --- a/drivers/isdn/hisax/hfc_pci.c
> > +++ b/drivers/isdn/hisax/hfc_pci.c
> > @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
> >         Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
> >
> >         /* Clear already pending ints */
> > -       if (Read_hfc(cs, HFCPCI_INT_S1));
> > +       (void) Read_hfc(cs, HFCPCI_INT_S1);
> 
> 
> 
> Why is this '(void)' necessary?
> 

It's not. I never tested without it since last time I tried to remove
the if statement, GCC complained at me but that was before the readb
change like you suggested.

I will go ahead and send a v2 with that cleaned up.

> I see no warning without it.
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Thanks for the review!
Nathan

[PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements

[Nathan Chancellor]

Clang warns:

drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
[-Werror,-Wempty-body]
        if (Read_hfc(cs, HFCPCI_INT_S1));
                                        ^
drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
separate line to silence this warning

In my attempt to hide the warnings because I thought they didn't serve
any purpose[1], Masahiro Yamada pointed out that {Read,Write}_hfc in
hci_pci.c should be using a standard register access method; otherwise,
the compiler will just remove the if statements.

For hfc_pci, use the versions of {Read,Write}_hfc found in
drivers/isdn/hardware/mISDN/hfc_pCI.h then remove the empty if statements.

For hfc_sx, {Read,Write}_hfc are already use a proper register accessor
(inb, outb) so just remove the unnecessary if statements.

[1]: https://lore.kernel.org/lkml/20181016021454.11953-1-natechancellor@gmail.com/

Link: https://github.com/ClangBuiltLinux/linux/issues/66
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Remove unnecessary cast to void, just remove if statements (thanks to
  review from Masahiro).

* Clean up commit message.

 drivers/isdn/hisax/hfc_pci.c | 6 +++---
 drivers/isdn/hisax/hfc_pci.h | 4 ++--
 drivers/isdn/hisax/hfc_sx.c  | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index 8e5b03161b2f..7bcd104e9dfe 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
 	Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
 
 	/* Clear already pending ints */
-	if (Read_hfc(cs, HFCPCI_INT_S1));
+	Read_hfc(cs, HFCPCI_INT_S1);
 
 	Write_hfc(cs, HFCPCI_STATES, HFCPCI_LOAD_STATE | 2);	/* HFC ST 2 */
 	udelay(10);
@@ -158,7 +158,7 @@ reset_hfcpci(struct IsdnCardState *cs)
 	/* Finally enable IRQ output */
 	cs->hw.hfcpci.int_m2 = HFCPCI_IRQ_ENABLE;
 	Write_hfc(cs, HFCPCI_INT_M2, cs->hw.hfcpci.int_m2);
-	if (Read_hfc(cs, HFCPCI_INT_S1));
+	Read_hfc(cs, HFCPCI_INT_S1);
 }
 
 /***************************************************/
@@ -1537,7 +1537,7 @@ hfcpci_bh(struct work_struct *work)
 					cs->hw.hfcpci.int_m1 &= ~HFCPCI_INTS_TIMER;
 					Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
 					/* Clear already pending ints */
-					if (Read_hfc(cs, HFCPCI_INT_S1));
+					Read_hfc(cs, HFCPCI_INT_S1);
 					Write_hfc(cs, HFCPCI_STATES, 4 | HFCPCI_LOAD_STATE);
 					udelay(10);
 					Write_hfc(cs, HFCPCI_STATES, 4);
diff --git a/drivers/isdn/hisax/hfc_pci.h b/drivers/isdn/hisax/hfc_pci.h
index 4e58700a3e61..4c3b3ba35726 100644
--- a/drivers/isdn/hisax/hfc_pci.h
+++ b/drivers/isdn/hisax/hfc_pci.h
@@ -228,8 +228,8 @@ typedef union {
 } fifo_area;
 
 
-#define Write_hfc(a, b, c) (*(((u_char *)a->hw.hfcpci.pci_io) + b) = c)
-#define Read_hfc(a, b) (*(((u_char *)a->hw.hfcpci.pci_io) + b))
+#define Write_hfc(a, b, c) (writeb(c, (a->hw.hfcpci.pci_io) + b))
+#define Read_hfc(a, b) (readb((a->hw.hfcpci.pci_io) + b))
 
 extern void main_irq_hcpci(struct BCState *bcs);
 extern void releasehfcpci(struct IsdnCardState *cs);
diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c
index 4d3b4b2f2612..12af628d9b2c 100644
--- a/drivers/isdn/hisax/hfc_sx.c
+++ b/drivers/isdn/hisax/hfc_sx.c
@@ -381,7 +381,7 @@ reset_hfcsx(struct IsdnCardState *cs)
 	Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
 
 	/* Clear already pending ints */
-	if (Read_hfc(cs, HFCSX_INT_S1));
+	Read_hfc(cs, HFCSX_INT_S1);
 
 	Write_hfc(cs, HFCSX_STATES, HFCSX_LOAD_STATE | 2);	/* HFC ST 2 */
 	udelay(10);
@@ -411,7 +411,7 @@ reset_hfcsx(struct IsdnCardState *cs)
 	/* Finally enable IRQ output */
 	cs->hw.hfcsx.int_m2 = HFCSX_IRQ_ENABLE;
 	Write_hfc(cs, HFCSX_INT_M2, cs->hw.hfcsx.int_m2);
-	if (Read_hfc(cs, HFCSX_INT_S2));
+	Read_hfc(cs, HFCSX_INT_S2);
 }
 
 /***************************************************/
@@ -1288,7 +1288,7 @@ hfcsx_bh(struct work_struct *work)
 					cs->hw.hfcsx.int_m1 &= ~HFCSX_INTS_TIMER;
 					Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
 					/* Clear already pending ints */
-					if (Read_hfc(cs, HFCSX_INT_S1));
+					Read_hfc(cs, HFCSX_INT_S1);
 
 					Write_hfc(cs, HFCSX_STATES, 4 | HFCSX_LOAD_STATE);
 					udelay(10);
-- 
2.19.1

Re: [PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements

[David Miller]

From: Nathan Chancellor <natechancellor@gmail.com>
Date: Wed, 17 Oct 2018 20:49:36 -0700

> @@ -228,8 +228,8 @@ typedef union {
>  } fifo_area;
>  
>  
> -#define Write_hfc(a, b, c) (*(((u_char *)a->hw.hfcpci.pci_io) + b) = c)
> -#define Read_hfc(a, b) (*(((u_char *)a->hw.hfcpci.pci_io) + b))
> +#define Write_hfc(a, b, c) (writeb(c, (a->hw.hfcpci.pci_io) + b))
> +#define Read_hfc(a, b) (readb((a->hw.hfcpci.pci_io) + b))

This will add new kinds of warnings.

The problem is that readb/writeb/etc. take an __iomem pointer, but pci_io
is declared as plain "unsigned char *".  It should be something like
"void * __iomem" of similar.

Re: [PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements

[Nathan Chancellor]

On Thu, Oct 18, 2018 at 03:42:19PM -0700, David Miller wrote:
> From: Nathan Chancellor <natechancellor@gmail.com>
> Date: Wed, 17 Oct 2018 20:49:36 -0700
> 
> > @@ -228,8 +228,8 @@ typedef union {
> >  } fifo_area;
> >  
> >  
> > -#define Write_hfc(a, b, c) (*(((u_char *)a->hw.hfcpci.pci_io) + b) = c)
> > -#define Read_hfc(a, b) (*(((u_char *)a->hw.hfcpci.pci_io) + b))
> > +#define Write_hfc(a, b, c) (writeb(c, (a->hw.hfcpci.pci_io) + b))
> > +#define Read_hfc(a, b) (readb((a->hw.hfcpci.pci_io) + b))
> 
> This will add new kinds of warnings.
> 
> The problem is that readb/writeb/etc. take an __iomem pointer, but pci_io
> is declared as plain "unsigned char *".  It should be something like
> "void * __iomem" of similar.

Thanks for the review, I went ahead and compiled with the following diff
on top of v2 and got no warnings from Clang, GCC, or sparse, does this
seem satisfactory for v3?

Nathan

========================================================================

diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index 7bcd104e9dfe..3dbaee8c604f 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -86,7 +86,7 @@ release_io_hfcpci(struct IsdnCardState *cs)
        pci_free_consistent(cs->hw.hfcpci.dev, 0x8000,
                            cs->hw.hfcpci.fifos, cs->hw.hfcpci.dma);
        cs->hw.hfcpci.fifos = NULL;
-       iounmap((void *)cs->hw.hfcpci.pci_io);
+       iounmap(cs->hw.hfcpci.pci_io);
 }
 
 /********************************************************************************/
@@ -1692,7 +1692,7 @@ setup_hfcpci(struct IsdnCard *card)
                printk(KERN_WARNING "HFC-PCI: No IRQ for PCI card found\n");
                return (0);
        }
-       cs->hw.hfcpci.pci_io = (char *)(unsigned long)dev_hfcpci->resource[1].start;
+       cs->hw.hfcpci.pci_io = (void __iomem *)(unsigned long)dev_hfcpci->resource[1].start;
        printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);
 
        if (!cs->hw.hfcpci.pci_io) {
diff --git a/drivers/isdn/hisax/hisax.h b/drivers/isdn/hisax/hisax.h
index 338d0408b377..40080e06421c 100644
--- a/drivers/isdn/hisax/hisax.h
+++ b/drivers/isdn/hisax/hisax.h
@@ -703,7 +703,7 @@ struct hfcPCI_hw {
        unsigned char nt_mode;
        int nt_timer;
        struct pci_dev *dev;
-       unsigned char *pci_io; /* start of PCI IO memory */
+       void __iomem *pci_io; /* start of PCI IO memory */
        dma_addr_t dma; /* dma handle for Fifos */
        void *fifos; /* FIFO memory */
        int last_bfifo_cnt[2]; /* marker saving last b-fifo frame count */

Re: [PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements

[David Miller]

From: Nathan Chancellor <natechancellor@gmail.com>
Date: Thu, 18 Oct 2018 17:21:17 -0700

> Thanks for the review, I went ahead and compiled with the following diff
> on top of v2 and got no warnings from Clang, GCC, or sparse, does this
> seem satisfactory for v3?

Well, one thing I notice.

> @@ -86,7 +86,7 @@ release_io_hfcpci(struct IsdnCardState *cs)
>         pci_free_consistent(cs->hw.hfcpci.dev, 0x8000,
>                             cs->hw.hfcpci.fifos, cs->hw.hfcpci.dma);
>         cs->hw.hfcpci.fifos = NULL;
> -       iounmap((void *)cs->hw.hfcpci.pci_io);
> +       iounmap(cs->hw.hfcpci.pci_io);
>  }

Driver uses iounmap().

> @@ -1692,7 +1692,7 @@ setup_hfcpci(struct IsdnCard *card)
>                 printk(KERN_WARNING "HFC-PCI: No IRQ for PCI card found\n");
>                 return (0);
>         }
> -       cs->hw.hfcpci.pci_io = (char *)(unsigned long)dev_hfcpci->resource[1].start;
> +       cs->hw.hfcpci.pci_io = (void __iomem *)(unsigned long)dev_hfcpci->resource[1].start;
>         printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);

But does not use iomap().  You won't need any cast here if it did use
iomap() properly.

Thanks.

Re: [PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements

[Nathan Chancellor]

On Thu, Oct 18, 2018 at 05:23:10PM -0700, David Miller wrote:
> From: Nathan Chancellor <natechancellor@gmail.com>
> Date: Thu, 18 Oct 2018 17:21:17 -0700
> 
> > Thanks for the review, I went ahead and compiled with the following diff
> > on top of v2 and got no warnings from Clang, GCC, or sparse, does this
> > seem satisfactory for v3?
> 
> Well, one thing I notice.
> 

> > @@ -86,7 +86,7 @@ release_io_hfcpci(struct IsdnCardState *cs)
> >         pci_free_consistent(cs->hw.hfcpci.dev, 0x8000,
> >                             cs->hw.hfcpci.fifos, cs->hw.hfcpci.dma);
> >         cs->hw.hfcpci.fifos = NULL;
> > -       iounmap((void *)cs->hw.hfcpci.pci_io);
> > +       iounmap(cs->hw.hfcpci.pci_io);
> >  }
> 
> Driver uses iounmap().
> 
> > @@ -1692,7 +1692,7 @@ setup_hfcpci(struct IsdnCard *card)
> >                 printk(KERN_WARNING "HFC-PCI: No IRQ for PCI card found\n");
> >                 return (0);
> >         }
> > -       cs->hw.hfcpci.pci_io = (char *)(unsigned long)dev_hfcpci->resource[1].start;
> > +       cs->hw.hfcpci.pci_io = (void __iomem *)(unsigned long)dev_hfcpci->resource[1].start;
> >         printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);
> 
> But does not use iomap().  You won't need any cast here if it did use
> iomap() properly.
> 
> Thanks.

So this?

diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index 3dbaee8c604f..ea0e4c6de3fb 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -1692,7 +1692,7 @@ setup_hfcpci(struct IsdnCard *card)
                printk(KERN_WARNING "HFC-PCI: No IRQ for PCI card found\n");
                return (0);
        }
-       cs->hw.hfcpci.pci_io = (void __iomem *)(unsigned long)dev_hfcpci->resource[1].start;
+       cs->hw.hfcpci.pci_io = ioremap(dev_hfcpci->resource[1].start, 256);
        printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);
 
        if (!cs->hw.hfcpci.pci_io) {
@@ -1716,7 +1716,6 @@ setup_hfcpci(struct IsdnCard *card)
                return 0;
        }
        pci_write_config_dword(cs->hw.hfcpci.dev, 0x80, (u32)cs->hw.hfcpci.dma);
-       cs->hw.hfcpci.pci_io = ioremap((ulong) cs->hw.hfcpci.pci_io, 256);
        printk(KERN_INFO
               "HFC-PCI: defined at mem %p fifo %p(%lx) IRQ %d HZ %d\n",
               cs->hw.hfcpci.pci_io,



Full context before the above diff should it be needed:


        cs->hw.hfcpci.pci_io = (void *)(unsigned long)dev_hfcpci->resource[1].start;
        printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);

        if (!cs->hw.hfcpci.pci_io) {
                printk(KERN_WARNING "HFC-PCI: No IO-Mem for PCI card found\n");
                return (0);
        }

        /* Allocate memory for FIFOS */
        cs->hw.hfcpci.fifos = pci_alloc_consistent(cs->hw.hfcpci.dev,
                                                   0x8000, &cs->hw.hfcpci.dma);
        if (!cs->hw.hfcpci.fifos) {
                printk(KERN_WARNING "HFC-PCI: Error allocating FIFO memory!\n");
                return 0;
        }
        if (cs->hw.hfcpci.dma & 0x7fff) {
                printk(KERN_WARNING
                       "HFC-PCI: Error DMA memory not on 32K boundary (%lx)\n",
                       (u_long)cs->hw.hfcpci.dma);
                pci_free_consistent(cs->hw.hfcpci.dev, 0x8000,
                                    cs->hw.hfcpci.fifos, cs->hw.hfcpci.dma);
                return 0;
        }
        pci_write_config_dword(cs->hw.hfcpci.dev, 0x80, (u32)cs->hw.hfcpci.dma);
        cs->hw.hfcpci.pci_io = ioremap((ulong) cs->hw.hfcpci.pci_io, 256);


Thanks,
Nathan

Re: [PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements

[David Miller]

From: Nathan Chancellor <natechancellor@gmail.com>
Date: Thu, 18 Oct 2018 17:42:51 -0700

> On Thu, Oct 18, 2018 at 05:23:10PM -0700, David Miller wrote:
>> From: Nathan Chancellor <natechancellor@gmail.com>
>> Date: Thu, 18 Oct 2018 17:21:17 -0700
>> 
>> > Thanks for the review, I went ahead and compiled with the following diff
>> > on top of v2 and got no warnings from Clang, GCC, or sparse, does this
>> > seem satisfactory for v3?
>> 
>> Well, one thing I notice.
>> 
> 
>> > @@ -86,7 +86,7 @@ release_io_hfcpci(struct IsdnCardState *cs)
>> >         pci_free_consistent(cs->hw.hfcpci.dev, 0x8000,
>> >                             cs->hw.hfcpci.fifos, cs->hw.hfcpci.dma);
>> >         cs->hw.hfcpci.fifos = NULL;
>> > -       iounmap((void *)cs->hw.hfcpci.pci_io);
>> > +       iounmap(cs->hw.hfcpci.pci_io);
>> >  }
>> 
>> Driver uses iounmap().
>> 
>> > @@ -1692,7 +1692,7 @@ setup_hfcpci(struct IsdnCard *card)
>> >                 printk(KERN_WARNING "HFC-PCI: No IRQ for PCI card found\n");
>> >                 return (0);
>> >         }
>> > -       cs->hw.hfcpci.pci_io = (char *)(unsigned long)dev_hfcpci->resource[1].start;
>> > +       cs->hw.hfcpci.pci_io = (void __iomem *)(unsigned long)dev_hfcpci->resource[1].start;
>> >         printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);
>> 
>> But does not use iomap().  You won't need any cast here if it did use
>> iomap() properly.
>> 
>> Thanks.
> 
> So this?

That's definitely a lot better, yes!

I wonder what exactly it is checking there though.  I guess it wants to see if the
resource->start value is zero and bail with an error it so.

Anyways, this code has been like this for ages and what you are proposing is
definitely a significant improvement.

Thanks.

Re: [PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements

[Nathan Chancellor]

On Thu, Oct 18, 2018 at 05:50:58PM -0700, David Miller wrote:
> From: Nathan Chancellor <natechancellor@gmail.com>
> Date: Thu, 18 Oct 2018 17:42:51 -0700
> 
> > On Thu, Oct 18, 2018 at 05:23:10PM -0700, David Miller wrote:
> >> From: Nathan Chancellor <natechancellor@gmail.com>
> >> Date: Thu, 18 Oct 2018 17:21:17 -0700
> >> 
> >> > Thanks for the review, I went ahead and compiled with the following diff
> >> > on top of v2 and got no warnings from Clang, GCC, or sparse, does this
> >> > seem satisfactory for v3?
> >> 
> >> Well, one thing I notice.
> >> 
> > 
> >> > @@ -86,7 +86,7 @@ release_io_hfcpci(struct IsdnCardState *cs)
> >> >         pci_free_consistent(cs->hw.hfcpci.dev, 0x8000,
> >> >                             cs->hw.hfcpci.fifos, cs->hw.hfcpci.dma);
> >> >         cs->hw.hfcpci.fifos = NULL;
> >> > -       iounmap((void *)cs->hw.hfcpci.pci_io);
> >> > +       iounmap(cs->hw.hfcpci.pci_io);
> >> >  }
> >> 
> >> Driver uses iounmap().
> >> 
> >> > @@ -1692,7 +1692,7 @@ setup_hfcpci(struct IsdnCard *card)
> >> >                 printk(KERN_WARNING "HFC-PCI: No IRQ for PCI card found\n");
> >> >                 return (0);
> >> >         }
> >> > -       cs->hw.hfcpci.pci_io = (char *)(unsigned long)dev_hfcpci->resource[1].start;
> >> > +       cs->hw.hfcpci.pci_io = (void __iomem *)(unsigned long)dev_hfcpci->resource[1].start;
> >> >         printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);
> >> 
> >> But does not use iomap().  You won't need any cast here if it did use
> >> iomap() properly.
> >> 
> >> Thanks.
> > 
> > So this?
> 
> That's definitely a lot better, yes!
> 
> I wonder what exactly it is checking there though.  I guess it wants to see if the
> resource->start value is zero and bail with an error it so.
> 
> Anyways, this code has been like this for ages and what you are proposing is
> definitely a significant improvement.
> 
> Thanks.

I was thinking the same thing. I think ioremap will still error out if
start is zero so this should be fine. I'll roll all of this up into v3,
thanks a lot for the review!

Nathan

[PATCH v3] isdn: hfc_{pci,sx}: Avoid empty body if statements

[Nathan Chancellor]

Clang warns:

drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
[-Werror,-Wempty-body]
        if (Read_hfc(cs, HFCPCI_INT_S1));
                                        ^
drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
separate line to silence this warning

In my attempt to hide the warnings because I thought they didn't serve
any purpose[1], Masahiro Yamada pointed out that {Read,Write}_hfc in
hci_pci.c should be using a standard register access method; otherwise,
the compiler will just remove the if statements.

For hfc_pci, use the versions of {Read,Write}_hfc found in
drivers/isdn/hardware/mISDN/hfc_pCI.h while converting pci_io to be
'void __iomem *' (and clean up ioremap) then remove the empty if
statements.

For hfc_sx, {Read,Write}_hfc are already use a proper register accessor
(inb, outb) so just remove the unnecessary if statements.

[1]: https://lore.kernel.org/lkml/20181016021454.11953-1-natechancellor@gmail.com/

Link: https://github.com/ClangBuiltLinux/linux/issues/66
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Remove unnecessary cast to void, just remove if statements (thanks to
  review from Masahiro).

* Clean up commit message.

v2 -> v3:

* Convert hfc_pci to properly use readb/writeb to avoid static checker
  issues.

 drivers/isdn/hisax/hfc_pci.c | 11 +++++------
 drivers/isdn/hisax/hfc_pci.h |  4 ++--
 drivers/isdn/hisax/hfc_sx.c  |  6 +++---
 drivers/isdn/hisax/hisax.h   |  2 +-
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index 8e5b03161b2f..ea0e4c6de3fb 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -86,7 +86,7 @@ release_io_hfcpci(struct IsdnCardState *cs)
 	pci_free_consistent(cs->hw.hfcpci.dev, 0x8000,
 			    cs->hw.hfcpci.fifos, cs->hw.hfcpci.dma);
 	cs->hw.hfcpci.fifos = NULL;
-	iounmap((void *)cs->hw.hfcpci.pci_io);
+	iounmap(cs->hw.hfcpci.pci_io);
 }
 
 /********************************************************************************/
@@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
 	Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
 
 	/* Clear already pending ints */
-	if (Read_hfc(cs, HFCPCI_INT_S1));
+	Read_hfc(cs, HFCPCI_INT_S1);
 
 	Write_hfc(cs, HFCPCI_STATES, HFCPCI_LOAD_STATE | 2);	/* HFC ST 2 */
 	udelay(10);
@@ -158,7 +158,7 @@ reset_hfcpci(struct IsdnCardState *cs)
 	/* Finally enable IRQ output */
 	cs->hw.hfcpci.int_m2 = HFCPCI_IRQ_ENABLE;
 	Write_hfc(cs, HFCPCI_INT_M2, cs->hw.hfcpci.int_m2);
-	if (Read_hfc(cs, HFCPCI_INT_S1));
+	Read_hfc(cs, HFCPCI_INT_S1);
 }
 
 /***************************************************/
@@ -1537,7 +1537,7 @@ hfcpci_bh(struct work_struct *work)
 					cs->hw.hfcpci.int_m1 &= ~HFCPCI_INTS_TIMER;
 					Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
 					/* Clear already pending ints */
-					if (Read_hfc(cs, HFCPCI_INT_S1));
+					Read_hfc(cs, HFCPCI_INT_S1);
 					Write_hfc(cs, HFCPCI_STATES, 4 | HFCPCI_LOAD_STATE);
 					udelay(10);
 					Write_hfc(cs, HFCPCI_STATES, 4);
@@ -1692,7 +1692,7 @@ setup_hfcpci(struct IsdnCard *card)
 		printk(KERN_WARNING "HFC-PCI: No IRQ for PCI card found\n");
 		return (0);
 	}
-	cs->hw.hfcpci.pci_io = (char *)(unsigned long)dev_hfcpci->resource[1].start;
+	cs->hw.hfcpci.pci_io = ioremap(dev_hfcpci->resource[1].start, 256);
 	printk(KERN_INFO "HiSax: HFC-PCI card manufacturer: %s card name: %s\n", id_list[i].vendor_name, id_list[i].card_name);
 
 	if (!cs->hw.hfcpci.pci_io) {
@@ -1716,7 +1716,6 @@ setup_hfcpci(struct IsdnCard *card)
 		return 0;
 	}
 	pci_write_config_dword(cs->hw.hfcpci.dev, 0x80, (u32)cs->hw.hfcpci.dma);
-	cs->hw.hfcpci.pci_io = ioremap((ulong) cs->hw.hfcpci.pci_io, 256);
 	printk(KERN_INFO
 	       "HFC-PCI: defined at mem %p fifo %p(%lx) IRQ %d HZ %d\n",
 	       cs->hw.hfcpci.pci_io,
diff --git a/drivers/isdn/hisax/hfc_pci.h b/drivers/isdn/hisax/hfc_pci.h
index 4e58700a3e61..4c3b3ba35726 100644
--- a/drivers/isdn/hisax/hfc_pci.h
+++ b/drivers/isdn/hisax/hfc_pci.h
@@ -228,8 +228,8 @@ typedef union {
 } fifo_area;
 
 
-#define Write_hfc(a, b, c) (*(((u_char *)a->hw.hfcpci.pci_io) + b) = c)
-#define Read_hfc(a, b) (*(((u_char *)a->hw.hfcpci.pci_io) + b))
+#define Write_hfc(a, b, c) (writeb(c, (a->hw.hfcpci.pci_io) + b))
+#define Read_hfc(a, b) (readb((a->hw.hfcpci.pci_io) + b))
 
 extern void main_irq_hcpci(struct BCState *bcs);
 extern void releasehfcpci(struct IsdnCardState *cs);
diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c
index 4d3b4b2f2612..12af628d9b2c 100644
--- a/drivers/isdn/hisax/hfc_sx.c
+++ b/drivers/isdn/hisax/hfc_sx.c
@@ -381,7 +381,7 @@ reset_hfcsx(struct IsdnCardState *cs)
 	Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
 
 	/* Clear already pending ints */
-	if (Read_hfc(cs, HFCSX_INT_S1));
+	Read_hfc(cs, HFCSX_INT_S1);
 
 	Write_hfc(cs, HFCSX_STATES, HFCSX_LOAD_STATE | 2);	/* HFC ST 2 */
 	udelay(10);
@@ -411,7 +411,7 @@ reset_hfcsx(struct IsdnCardState *cs)
 	/* Finally enable IRQ output */
 	cs->hw.hfcsx.int_m2 = HFCSX_IRQ_ENABLE;
 	Write_hfc(cs, HFCSX_INT_M2, cs->hw.hfcsx.int_m2);
-	if (Read_hfc(cs, HFCSX_INT_S2));
+	Read_hfc(cs, HFCSX_INT_S2);
 }
 
 /***************************************************/
@@ -1288,7 +1288,7 @@ hfcsx_bh(struct work_struct *work)
 					cs->hw.hfcsx.int_m1 &= ~HFCSX_INTS_TIMER;
 					Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
 					/* Clear already pending ints */
-					if (Read_hfc(cs, HFCSX_INT_S1));
+					Read_hfc(cs, HFCSX_INT_S1);
 
 					Write_hfc(cs, HFCSX_STATES, 4 | HFCSX_LOAD_STATE);
 					udelay(10);
diff --git a/drivers/isdn/hisax/hisax.h b/drivers/isdn/hisax/hisax.h
index 338d0408b377..40080e06421c 100644
--- a/drivers/isdn/hisax/hisax.h
+++ b/drivers/isdn/hisax/hisax.h
@@ -703,7 +703,7 @@ struct hfcPCI_hw {
 	unsigned char nt_mode;
 	int nt_timer;
 	struct pci_dev *dev;
-	unsigned char *pci_io; /* start of PCI IO memory */
+	void __iomem *pci_io; /* start of PCI IO memory */
 	dma_addr_t dma; /* dma handle for Fifos */
 	void *fifos; /* FIFO memory */
 	int last_bfifo_cnt[2]; /* marker saving last b-fifo frame count */
-- 
2.19.1

Re: [PATCH v3] isdn: hfc_{pci,sx}: Avoid empty body if statements

[David Miller]

From: Nathan Chancellor <natechancellor@gmail.com>
Date: Thu, 18 Oct 2018 18:11:04 -0700

> Clang warns:
> 
> drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
> [-Werror,-Wempty-body]
>         if (Read_hfc(cs, HFCPCI_INT_S1));
>                                         ^
> drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
> separate line to silence this warning
> 
> In my attempt to hide the warnings because I thought they didn't serve
> any purpose[1], Masahiro Yamada pointed out that {Read,Write}_hfc in
> hci_pci.c should be using a standard register access method; otherwise,
> the compiler will just remove the if statements.
> 
> For hfc_pci, use the versions of {Read,Write}_hfc found in
> drivers/isdn/hardware/mISDN/hfc_pCI.h while converting pci_io to be
> 'void __iomem *' (and clean up ioremap) then remove the empty if
> statements.
> 
> For hfc_sx, {Read,Write}_hfc are already use a proper register accessor
> (inb, outb) so just remove the unnecessary if statements.
> 
> [1]: https://lore.kernel.org/lkml/20181016021454.11953-1-natechancellor@gmail.com/
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/66
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Applied.