Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1423438 > unrolled thread
| Started by | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| First post | 2016-06-15 22:50 +0200 |
| Last post | 2016-06-16 09:40 +0200 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 1/2] scsi: lpfc: avoid harmless comparison warning Arnd Bergmann <arnd@arndb.de> - 2016-06-15 22:50 +0200
[PATCH 2/2] scsi: wd7000: print sector number as 64-bit Arnd Bergmann <arnd@arndb.de> - 2016-06-15 23:00 +0200
Re: [PATCH 2/2] scsi: wd7000: print sector number as 64-bit "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-06-21 03:20 +0200
Re: [PATCH 2/2] scsi: wd7000: print sector number as 64-bit Arnd Bergmann <arnd@arndb.de> - 2016-06-21 13:50 +0200
Re: [PATCH 1/2] scsi: lpfc: avoid harmless comparison warning Johannes Thumshirn <jthumshirn@suse.de> - 2016-06-16 09:40 +0200
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-06-15 22:50 +0200 |
| Subject | [PATCH 1/2] scsi: lpfc: avoid harmless comparison warning |
| Message-ID | <rKpV7-7iY-15@gated-at.bofh.it> |
When building with -Wextra, we get a lot of warnings for the lpfc driver
concerning expressions that are always true, starting with:
drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_enable_npiv_init':
drivers/scsi/lpfc/lpfc_attr.c:2786:77: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_enable_rrq_init':
drivers/scsi/lpfc/lpfc_attr.c:2802:76: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_suppress_link_up_init':
drivers/scsi/lpfc/lpfc_attr.c:2812:2050: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_log_verbose_init':
drivers/scsi/lpfc/lpfc_attr.c:3064:1930: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
The code works as intented, but it would be nice to shut up the
warning so we don't clutter up build logs with this. Using a
separate inline function for it makes it clear to the compiler
that the comparison is necessary in the caller but still lets
it do the constant-folding.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/scsi/lpfc/lpfc_attr.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index cfec2eca4dd3..3e1d2e669902 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -1620,6 +1620,11 @@ lpfc_sriov_hw_max_virtfn_show(struct device *dev,
return snprintf(buf, PAGE_SIZE, "%d\n", max_nr_virtfn);
}
+static inline bool lpfc_rangecheck(uint val, uint min, uint max)
+{
+ return val >= min && val <= max;
+}
+
/**
* lpfc_param_show - Return a cfg attribute value in decimal
*
@@ -1697,7 +1702,7 @@ lpfc_##attr##_show(struct device *dev, struct device_attribute *attr, \
static int \
lpfc_##attr##_init(struct lpfc_hba *phba, uint val) \
{ \
- if (val >= minval && val <= maxval) {\
+ if (lpfc_rangecheck(val, minval, maxval)) {\
phba->cfg_##attr = val;\
return 0;\
}\
@@ -1732,7 +1737,7 @@ lpfc_##attr##_init(struct lpfc_hba *phba, uint val) \
static int \
lpfc_##attr##_set(struct lpfc_hba *phba, uint val) \
{ \
- if (val >= minval && val <= maxval) {\
+ if (lpfc_rangecheck(val, minval, maxval)) {\
lpfc_printf_log(phba, KERN_ERR, LOG_INIT, \
"3052 lpfc_" #attr " changed from %d to %d\n", \
phba->cfg_##attr, val); \
@@ -1856,7 +1861,7 @@ lpfc_##attr##_show(struct device *dev, struct device_attribute *attr, \
static int \
lpfc_##attr##_init(struct lpfc_vport *vport, uint val) \
{ \
- if (val >= minval && val <= maxval) {\
+ if (lpfc_rangecheck(val, minval, maxval)) {\
vport->cfg_##attr = val;\
return 0;\
}\
@@ -1888,7 +1893,7 @@ lpfc_##attr##_init(struct lpfc_vport *vport, uint val) \
static int \
lpfc_##attr##_set(struct lpfc_vport *vport, uint val) \
{ \
- if (val >= minval && val <= maxval) {\
+ if (lpfc_rangecheck(val, minval, maxval)) {\
lpfc_printf_vlog(vport, KERN_ERR, LOG_INIT, \
"3053 lpfc_" #attr \
" changed from %d (x%x) to %d (x%x)\n", \
--
2.9.0
[toc] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-06-15 23:00 +0200 |
| Subject | [PATCH 2/2] scsi: wd7000: print sector number as 64-bit |
| Message-ID | <rKq4N-7mg-3@gated-at.bofh.it> |
| In reply to | #1423438 |
Enabling format checking in dprintk() shows that wd7000_biosparam
uses an incorrect format string for sector_t:
drivers/scsi/wd7000.c: In function 'wd7000_biosparam':
drivers/scsi/wd7000.c:1594:21: error: format '%d' expects argument of type 'int', but argument 3 has type 'sector_t {aka long long unsigned int}' [-Werror=format=]
As sector_t can be 32-bit wide, this adds a cast to 'u64' and prints
that with the correct format. The change to use no_printk()
generally helps with finding this kind of hidden format string bug,
and I found that when building with "-Wextra", which warned about
an empty else clause in
} else
dprintk("ok!\n");
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Not Cc'ing Miroslav Zagorac <zaga@fly.cc.fer.hr>, that address bounces
with "Requested action not taken: mailbox unavailable invalid DNS MX or A/AAAA resource record"
diff --git a/drivers/scsi/wd7000.c b/drivers/scsi/wd7000.c
index 0c0f17b9a3eb..39a3b5333bd6 100644
--- a/drivers/scsi/wd7000.c
+++ b/drivers/scsi/wd7000.c
@@ -192,7 +192,7 @@
#ifdef WD7000_DEBUG
#define dprintk printk
#else
-#define dprintk(format,args...)
+#define dprintk no_printk
#endif
/*
@@ -1591,8 +1591,8 @@ static int wd7000_biosparam(struct scsi_device *sdev,
{
char b[BDEVNAME_SIZE];
- dprintk("wd7000_biosparam: dev=%s, size=%d, ",
- bdevname(bdev, b), capacity);
+ dprintk("wd7000_biosparam: dev=%s, size=%lld, ",
+ bdevname(bdev, b), (s64)capacity);
(void)b; /* unused var warning? */
/*
[toc] | [prev] | [next] | [standalone]
| From | "Martin K. Petersen" <martin.petersen@oracle.com> |
|---|---|
| Date | 2016-06-21 03:20 +0200 |
| Subject | Re: [PATCH 2/2] scsi: wd7000: print sector number as 64-bit |
| Message-ID | <rMiw9-7nZ-1@gated-at.bofh.it> |
| In reply to | #1423441 |
>>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
As sector_t can be 32-bit wide, this adds a cast to 'u64' and prints
that with the correct format. The change to use no_printk()
[...]
+ dprintk("wd7000_biosparam: dev=%s, size=%lld, ",
+ bdevname(bdev, b), (s64)capacity);
s64?
Why not unsigned long long like we usually do with sector_t?
--
Martin K. Petersen Oracle Linux Engineering
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-06-21 13:50 +0200 |
| Subject | Re: [PATCH 2/2] scsi: wd7000: print sector number as 64-bit |
| Message-ID | <rMslQ-5hj-39@gated-at.bofh.it> |
| In reply to | #1427174 |
On Monday, June 20, 2016 9:12:50 PM CEST Martin K. Petersen wrote:
> >>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
>
> As sector_t can be 32-bit wide, this adds a cast to 'u64' and prints
> that with the correct format. The change to use no_printk()
>
> [...]
>
> + dprintk("wd7000_biosparam: dev=%s, size=%lld, ",
> + bdevname(bdev, b), (s64)capacity);
>
> s64?
>
> Why not unsigned long long like we usually do with sector_t?
The printk() was using %d as the format string, printing it
as a signed number, so I did not change it.
Using %llu makes sense though, I've resent it like that.
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Johannes Thumshirn <jthumshirn@suse.de> |
|---|---|
| Date | 2016-06-16 09:40 +0200 |
| Message-ID | <rKA49-5o9-3@gated-at.bofh.it> |
| In reply to | #1423438 |
On Wed, Jun 15, 2016 at 10:42:17PM +0200, Arnd Bergmann wrote: > When building with -Wextra, we get a lot of warnings for the lpfc driver > concerning expressions that are always true, starting with: > > drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_enable_npiv_init': > drivers/scsi/lpfc/lpfc_attr.c:2786:77: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] > drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_enable_rrq_init': > drivers/scsi/lpfc/lpfc_attr.c:2802:76: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] > drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_suppress_link_up_init': > drivers/scsi/lpfc/lpfc_attr.c:2812:2050: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] > drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_log_verbose_init': > drivers/scsi/lpfc/lpfc_attr.c:3064:1930: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] > > The code works as intented, but it would be nice to shut up the > warning so we don't clutter up build logs with this. Using a > separate inline function for it makes it clear to the compiler > that the comparison is necessary in the caller but still lets > it do the constant-folding. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web