[trivial PATCH V2] treewide: Align function definition open/close braces

[Joe Perches]

Some functions definitions have either the initial open brace and/or
the closing brace outside of column 1.

Move those braces to column 1.

This allows various function analyzers like gnu complexity to work
properly for these modified functions.

Signed-off-by: Joe Perches <j...@perches.com>
Acked-by: Andy Shevchenko <andy.shevche...@gmail.com>
Acked-by: Paul Moore <p...@paul-moore.com>
Acked-by: Alex Deucher <alexander.deuc...@amd.com>
Acked-by: Dave Chinner <dchin...@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.w...@oracle.com>
Acked-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>
Acked-by: Martin K. Petersen <martin.peter...@oracle.com>
Acked-by: Takashi Iwai <ti...@suse.de>
Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---

git diff -w still shows no difference.

This patch was sent but December and not applied.

As the trivial maintainer seems not active, it'd be nice if
Andrew Morton picks this up.

V2: Remove fs/xfs/libxfs/xfs_alloc.c as it's updated and remerge the rest

 arch/x86/include/asm/atomic64_32.h   |  2 +-
 drivers/acpi/custom_method.c |  2 +-
 drivers/acpi/fan.c   |  2 +-
 drivers/gpu/drm/amd/display/dc/core/dc.c |  2 +-
 drivers/media/i2c/msp3400-kthreads.c |  2 +-
 drivers/message/fusion/mptsas.c  |  2 +-
 drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c |  2 +-
 drivers/net/wireless/ath/ath9k/xmit.c|  2 +-
 drivers/platform/x86/eeepc-laptop.c  |  2 +-
 drivers/rtc/rtc-ab-b5ze-s3.c |  2 +-
 drivers/scsi/dpt_i2o.c   |  2 +-
 drivers/scsi/sym53c8xx_2/sym_glue.c  |  2 +-
 fs/locks.c   |  2 +-
 fs/ocfs2/stack_user.c|  2 +-
 fs/xfs/xfs_export.c  |  2 +-
 kernel/audit.c   |  6 +++---
 kernel/trace/trace_printk.c  |  4 ++--
 lib/raid6/sse2.c | 14 +++---
 sound/soc/fsl/fsl_dma.c  |  2 +-
 19 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/atomic64_32.h 
b/arch/x86/include/asm/atomic64_32.h
index 46e1ef17d92d..92212bf0484f 100644
--- a/arch/x86/include/asm/atomic64_32.h
+++ b/arch/x86/include/asm/atomic64_32.h
@@ -123,7 +123,7 @@ static inline long long arch_atomic64_read(const atomic64_t 
*v)
long long r;
alternative_atomic64(read, "=" (r), "c" (v) : "memory");
return r;
- }
+}
 
 /**
  * arch_atomic64_add_return - add and return
diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index b33fba70ec51..a07fbe999eb6 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -97,7 +97,7 @@ static void __exit acpi_custom_method_exit(void)
 {
if (cm_dentry)
debugfs_remove(cm_dentry);
- }
+}
 
 module_init(acpi_custom_method_init);
 module_exit(acpi_custom_method_exit);
diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 6cf4988206f2..3563103590c6 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -219,7 +219,7 @@ fan_set_cur_state(struct thermal_cooling_device *cdev, 
unsigned long state)
return fan_set_state_acpi4(device, state);
else
return fan_set_state(device, state);
- }
+}
 
 static const struct thermal_cooling_device_ops fan_cooling_ops = {
.get_max_state = fan_get_max_state,
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 8394d69b963f..e934326a95d3 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -588,7 +588,7 @@ static void disable_dangling_plane(struct dc *dc, struct 
dc_state *context)
  
**/
 
 struct dc *dc_create(const struct dc_init_data *init_params)
- {
+{
struct dc *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
unsigned int full_pipe_count;
 
diff --git a/drivers/media/i2c/msp3400-kthreads.c 
b/drivers/media/i2c/msp3400-kthreads.c
index 4dd01e9f553b..dc6cb8d475b3 100644
--- a/drivers/media/i2c/msp3400-kthreads.c
+++ b/drivers/media/i2c/msp3400-kthreads.c
@@ -885,7 +885,7 @@ static int msp34xxg_modus(struct i2c_client *client)
 }
 
 static void msp34xxg_set_source(struct i2c_client *client, u16 reg, int in)
- {
+{
struct msp_state *state = to_state(i2c_get_clientdata(client));
int source, matrix;
 
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 439ee9c5f535..231f3a1e27bf 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -2967,7 +2967,7 @@ mp

Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW

[Joe Perches]

On Wed, 2017-12-20 at 10:59 +0100, Greg Kroah-Hartman wrote:
> > > Why you didn't send that patch to the sysfs maintainer is a bit odd...  :)
> > 
> > So here's an opportunity for you:
> > 
> > The sysfs maintainer hasn't added include/linux/sysfs.h to
> > the list of maintained files...
> > 
> > DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS
> > M:  Greg Kroah-Hartman 
> > T:  git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> > S:  Supported
> > F:  Documentation/kobject.txt
> > F:  drivers/base/
> > F:  fs/debugfs/
> > F:  fs/sysfs/
> > F:  include/linux/debugfs.h
> > F:  include/linux/kobj*
> > F:  lib/kobj*
> 
> Heh, good point, but using get_maintainer.pl does put me at the top of
> the list that you should be cc:ing:
> 
> $ ./scripts/get_maintainer.pl --file include/linux/sysfs.h
> Greg Kroah-Hartman  
> (commit_signer:3/3=100%,authored:2/3=67%,added_lines:7/8=88%)
> Kate Stewart  (commit_signer:1/3=33%)
> Thomas Gleixner  (commit_signer:1/3=33%)
> Philippe Ombredanne  (commit_signer:1/3=33%)
> Nick Desaulniers  
> (commit_signer:1/3=33%,authored:1/3=33%,added_lines:1/8=12%,removed_lines:1/1=100%)
> linux-ker...@vger.kernel.org (open list)

The script I use to send patches adds --nogit --nogit-fallback
to copy only listed maintainers because people that send cleanup
patches don't generally like to get random patches.

> > btw: there are many uses of a reversed declaration style of DEVICE_ATTR
> > 
> > Here's another thing that could be done for more DEVICE_ATTR_ uses.
> > 
> > ===
> > 
> > Some DEVICE_ATTR definitions use a reversed static function form from
> > the typical.  Convert them to use the more common macro form so it is
> > easier to grep for the style.
[]
> > $ git grep --name-only -w DEVICE_ATTR | \
> >   xargs perl -i dev_attr_rw_backwards.perl
> Ah, nice, I love perl :

That was a bad copy/paste of the script.

The actual script for RW is:

$ cat dev_attr_rw_backwards.perl
local $/;
while (<>) {
my $file = $_;
while ($file =~ m/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,/g) {
my $var = $1;
if ($file =~ 
s/\bDEVICE_ATTR\s*\(\s*${var}\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*show_${var}\s*,\s*store_${var}\s*\)/DEVICE_ATTR_RW(${var})/g)
 {
$file =~ s/\bshow_${var}\b/${var}_show/g;
$file =~ s/\bstore_${var}\b/${var}_store/g;
}
}
print $file;
}

There are 3 different perl scripts for rw, ro, and wo.
and these scripts, because of function renaming and
possible reuse of the original function names by other
string concatenated macros, create some bad conversions
so they need some manual cleanups too.

Perhaps coccinelle could do a better job of it, but
likely string concatenation macro uses are going to
be hard to deal with in any case.

Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW

[Joe Perches]

On Wed, 2017-12-20 at 10:32 +0100, Greg Kroah-Hartman wrote:
> On Wed, Dec 20, 2017 at 01:24:44AM -0800, Joe Perches wrote:
> > On Wed, 2017-12-20 at 10:34 +0200, Jarkko Nikula wrote:
> > > On Tue, Dec 19, 2017 at 10:15:07AM -0800, Joe Perches wrote:
> > > > Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.
> > 
> > [] 
> > > > diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c
> > 
> > []
> > > > @@ -854,7 +854,7 @@ static ssize_t dma_op_mode_store(struct device *dev,
> > > > return size;
> > > >  }
> > > >  
> > > > -static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, 
> > > > dma_op_mode_store);
> > > > +static DEVICE_ATTR_RW(dma_op_mode);
> > > >  
> > > 
> > > While I can ack this part here if it helps generic cleanup effort I
> > > don't understart would it improve code readability in general? Mode 644
> > > is clear and don't need any grepping but for DEVICE_ATTR_RW() I had to go
> > > through all of these files in order to see what does it mean:
> 
> Yeah, 644 is "clear", but _RW() is even more clear.  Ideally I want to
> get rid of all of the "non-standard" users that set random modes of
> sysfs files, as we get it wrong too many times.  Using the "defaults" is
> much better.
> 
> > Are you suggesting that device.h (as that is where
> > DEVICE_ATTR and the other DEVICE_ATTR_ variants
> > are #defined) should have better comments for the
> > _ variants?
> > 
> > > DEVICE_ATTR_RW: include/linux/device.h
> > > __ATTR_RW: include/linux/sysfs.h
> > > S_IWUSR: include/uapi/linux/stat.h
> > > S_IRUGO: include/linux/stat.h
> > 
> > btw: patch 1/4 of the series does remove the uses of
> > S_ from these macros in sysfs.h and converts
> > them to simple octal instead.
> 
> Why you didn't send that patch to the sysfs maintainer is a bit odd...  :)

So here's an opportunity for you:

The sysfs maintainer hasn't added include/linux/sysfs.h to
the list of maintained files...

DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS
M:  Greg Kroah-Hartman <gre...@linuxfoundation.org>
T:  git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
S:  Supported
F:  Documentation/kobject.txt
F:  drivers/base/
F:  fs/debugfs/
F:  fs/sysfs/
F:  include/linux/debugfs.h
F:  include/linux/kobj*
F:  lib/kobj*

> I should be taking this whole series through my tree.  Joe, thanks for
> doing this in an automated way, I've been wanting to see this done for a
> long time.

btw: there are many uses of a reversed declaration style of DEVICE_ATTR

Here's another thing that could be done for more DEVICE_ATTR_ uses.

===

Some DEVICE_ATTR definitions use a reversed static function form from
the typical.  Convert them to use the more common macro form so it is
easier to grep for the style.

i.e.:
static ssize_t show_(...)
and
static ssize_t store_(...)

where the static function names in the DEVICE_ATTR_RW macro are reversed

static ssize_t _show(...)
and
static ssize_t _store(...)

Done with perl script

$ cat dev_attr_rw_backwards.perl
local $/;
while (<>) {
my $file = $_;
while ($file =~ m/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,/g) {
my $var = $1;
if ($file =~ s/\bDEVICE_ATTR\s*\(\s*${var}\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S
$file =~ s/\bshow_${var}\b/${var}_show/g;
$file =~ s/\bstore_${var}\b/${var}_store/g;
}
}
print $file;
}

$ git grep --name-only -w DEVICE_ATTR | \
  xargs perl -i dev_attr_rw_backwards.perl

Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW

[Joe Perches]

On Wed, 2017-12-20 at 10:34 +0200, Jarkko Nikula wrote:
> On Tue, Dec 19, 2017 at 10:15:07AM -0800, Joe Perches wrote:
> > Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.
[] 
> > diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c
[]
> > @@ -854,7 +854,7 @@ static ssize_t dma_op_mode_store(struct device *dev,
> > return size;
> >  }
> >  
> > -static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, dma_op_mode_store);
> > +static DEVICE_ATTR_RW(dma_op_mode);
> >  
> 
> While I can ack this part here if it helps generic cleanup effort I
> don't understart would it improve code readability in general? Mode 644
> is clear and don't need any grepping but for DEVICE_ATTR_RW() I had to go
> through all of these files in order to see what does it mean:

Are you suggesting that device.h (as that is where
DEVICE_ATTR and the other DEVICE_ATTR_ variants
are #defined) should have better comments for the
_ variants?

> DEVICE_ATTR_RW: include/linux/device.h
> __ATTR_RW: include/linux/sysfs.h
> S_IWUSR: include/uapi/linux/stat.h
> S_IRUGO: include/linux/stat.h

btw: patch 1/4 of the series does remove the uses of
S_ from these macros in sysfs.h and converts
them to simple octal instead.

Re: [-next PATCH 4/4] treewide: Use DEVICE_ATTR_WO

[Joe Perches]

On Tue, 2017-12-19 at 19:44 +0100, Borislav Petkov wrote:
> On Tue, Dec 19, 2017 at 10:15:09AM -0800, Joe Perches wrote:
> > Convert DEVICE_ATTR uses to DEVICE_ATTR_WO where possible.
> > 
> > Done with perl script:
> > 
> > $ git grep -w --name-only DEVICE_ATTR | \
> >   xargs perl -i -e 'local $/; while (<>) { 
> > s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(?:\s*S_IWUSR\s*|\s*0200\s*)\)?\s*,\s*NULL\s*,\s*\s_store\s*\)/DEVICE_ATTR_WO(\1)/g;
> >  print;}'
[]
> > diff --git a/arch/x86/kernel/cpu/microcode/core.c 
> > b/arch/x86/kernel/cpu/microcode/core.c
[]
> > @@ -560,7 +560,7 @@ static ssize_t pf_show(struct device *dev,
> > return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
> >  }
> >  
> > -static DEVICE_ATTR(reload, 0200, NULL, reload_store);
> > +static DEVICE_ATTR_WO(reload);
> >  static DEVICE_ATTR(version, 0400, version_show, NULL);
> >  static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
> >  
> 
> # cat /sys/devices/system/cpu/microcode/reload
> cat: /sys/devices/system/cpu/microcode/reload: Permission denied

Not different behavior.  It was and remains write only.

> The reason for the code churn being?

Consistency for easier grep by use-type.

[-next PATCH 0/4] sysfs and DEVICE_ATTR_

[Joe Perches]

Joe Perches (4):
  sysfs.h: Use octal permissions
  treewide: Use DEVICE_ATTR_RW
  treewide: Use DEVICE_ATTR_RO
  treewide: Use DEVICE_ATTR_WO

 arch/arm/mach-pxa/sharpsl_pm.c |  4 +-
 arch/s390/kernel/smp.c |  2 +-
 arch/s390/kernel/topology.c|  3 +-
 arch/sh/drivers/push-switch.c  |  2 +-
 arch/tile/kernel/sysfs.c   | 12 ++--
 arch/x86/kernel/cpu/microcode/core.c   |  2 +-
 drivers/acpi/device_sysfs.c|  6 +-
 drivers/char/ipmi/ipmi_msghandler.c| 17 +++---
 drivers/gpu/drm/i915/i915_sysfs.c  | 12 ++--
 drivers/input/touchscreen/elants_i2c.c |  2 +-
 drivers/net/ethernet/ibm/ibmvnic.c |  2 +-
 drivers/net/wimax/i2400m/sysfs.c   |  3 +-
 drivers/nvme/host/core.c   | 10 ++--
 drivers/platform/x86/compal-laptop.c   | 18 ++
 drivers/s390/cio/css.c |  8 +--
 drivers/s390/cio/device.c  | 10 ++--
 drivers/s390/crypto/ap_card.c  |  2 +-
 drivers/scsi/hpsa.c| 10 ++--
 drivers/scsi/lpfc/lpfc_attr.c  | 64 --
 .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c   |  8 +--
 drivers/thermal/thermal_sysfs.c| 17 +++---
 drivers/tty/serial/sh-sci.c|  2 +-
 drivers/usb/host/xhci-dbgcap.c |  2 +-
 drivers/usb/phy/phy-tahvo.c|  2 +-
 drivers/video/fbdev/auo_k190x.c|  4 +-
 drivers/video/fbdev/w100fb.c   |  4 +-
 include/linux/sysfs.h  | 14 ++---
 lib/test_firmware.c| 14 ++---
 lib/test_kmod.c| 14 ++---
 sound/soc/omap/mcbsp.c |  4 +-
 sound/soc/soc-core.c   |  2 +-
 sound/soc/soc-dapm.c   |  2 +-
 32 files changed, 120 insertions(+), 158 deletions(-)

-- 
2.15.0

[-next PATCH 4/4] treewide: Use DEVICE_ATTR_WO

[Joe Perches]

Convert DEVICE_ATTR uses to DEVICE_ATTR_WO where possible.

Done with perl script:

$ git grep -w --name-only DEVICE_ATTR | \
  xargs perl -i -e 'local $/; while (<>) { 
s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(?:\s*S_IWUSR\s*|\s*0200\s*)\)?\s*,\s*NULL\s*,\s*\s_store\s*\)/DEVICE_ATTR_WO(\1)/g;
 print;}'

Signed-off-by: Joe Perches <j...@perches.com>
---
 arch/s390/kernel/smp.c | 2 +-
 arch/x86/kernel/cpu/microcode/core.c   | 2 +-
 drivers/input/touchscreen/elants_i2c.c | 2 +-
 drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
 drivers/net/wimax/i2400m/sysfs.c   | 3 +--
 drivers/scsi/lpfc/lpfc_attr.c  | 3 +--
 drivers/thermal/thermal_sysfs.c| 2 +-
 7 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index b8c1a85bcf2d..a919b2f0141d 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -1151,7 +1151,7 @@ static ssize_t __ref rescan_store(struct device *dev,
rc = smp_rescan_cpus();
return rc ? rc : count;
 }
-static DEVICE_ATTR(rescan, 0200, NULL, rescan_store);
+static DEVICE_ATTR_WO(rescan);
 #endif /* CONFIG_HOTPLUG_CPU */
 
 static int __init s390_smp_init(void)
diff --git a/arch/x86/kernel/cpu/microcode/core.c 
b/arch/x86/kernel/cpu/microcode/core.c
index c4fa4a85d4cb..09c74b0560dd 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -560,7 +560,7 @@ static ssize_t pf_show(struct device *dev,
return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
 }
 
-static DEVICE_ATTR(reload, 0200, NULL, reload_store);
+static DEVICE_ATTR_WO(reload);
 static DEVICE_ATTR(version, 0400, version_show, NULL);
 static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
 
diff --git a/drivers/input/touchscreen/elants_i2c.c 
b/drivers/input/touchscreen/elants_i2c.c
index a458e5ec9e41..819213e88f32 100644
--- a/drivers/input/touchscreen/elants_i2c.c
+++ b/drivers/input/touchscreen/elants_i2c.c
@@ -1000,7 +1000,7 @@ static ssize_t show_iap_mode(struct device *dev,
"Normal" : "Recovery");
 }
 
-static DEVICE_ATTR(calibrate, S_IWUSR, NULL, calibrate_store);
+static DEVICE_ATTR_WO(calibrate);
 static DEVICE_ATTR(iap_mode, S_IRUGO, show_iap_mode, NULL);
 static DEVICE_ATTR(update_fw, S_IWUSR, NULL, write_update_fw);
 
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
b/drivers/net/ethernet/ibm/ibmvnic.c
index 1dc4aef37d3a..42b96e1a1b13 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -4411,7 +4411,7 @@ static ssize_t failover_store(struct device *dev, struct 
device_attribute *attr,
return count;
 }
 
-static DEVICE_ATTR(failover, 0200, NULL, failover_store);
+static DEVICE_ATTR_WO(failover);
 
 static unsigned long ibmvnic_get_desired_dma(struct vio_dev *vdev)
 {
diff --git a/drivers/net/wimax/i2400m/sysfs.c b/drivers/net/wimax/i2400m/sysfs.c
index 1237109f251a..8c67df11105c 100644
--- a/drivers/net/wimax/i2400m/sysfs.c
+++ b/drivers/net/wimax/i2400m/sysfs.c
@@ -65,8 +65,7 @@ ssize_t i2400m_idle_timeout_store(struct device *dev,
 }
 
 static
-DEVICE_ATTR(i2400m_idle_timeout, S_IWUSR,
-   NULL, i2400m_idle_timeout_store);
+DEVICE_ATTR_WO(i2400m_idle_timeout);
 
 static
 struct attribute *i2400m_dev_attrs[] = {
diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index 517ff203cfde..6ddaf51a23f6 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -2418,8 +2418,7 @@ lpfc_soft_wwn_enable_store(struct device *dev, struct 
device_attribute *attr,
 
return count;
 }
-static DEVICE_ATTR(lpfc_soft_wwn_enable, S_IWUSR, NULL,
-  lpfc_soft_wwn_enable_store);
+static DEVICE_ATTR_WO(lpfc_soft_wwn_enable);
 
 /**
  * lpfc_soft_wwpn_show - Return the cfg soft ww port name of the adapter
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 2bc964392924..ba81c9080f6e 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -317,7 +317,7 @@ emul_temp_store(struct device *dev, struct device_attribute 
*attr,
 
return ret ? ret : count;
 }
-static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
+static DEVICE_ATTR_WO(emul_temp);
 #endif
 
 static ssize_t
-- 
2.15.0

[-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW

[Joe Perches]

Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.

Done with perl script:

$ git grep -w --name-only DEVICE_ATTR | \
  xargs perl -i -e 'local $/; while (<>) { 
s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*\1_show\s*,\s*\1_store\s*\)/DEVICE_ATTR_RW(\1)/g;
 print;}'

Signed-off-by: Joe Perches <j...@perches.com>
---
 arch/s390/kernel/topology.c  |  3 +--
 arch/tile/kernel/sysfs.c |  2 +-
 drivers/gpu/drm/i915/i915_sysfs.c|  6 ++---
 drivers/platform/x86/compal-laptop.c | 18 +--
 drivers/s390/cio/device.c|  2 +-
 drivers/scsi/lpfc/lpfc_attr.c| 43 
 drivers/thermal/thermal_sysfs.c  |  9 
 drivers/tty/serial/sh-sci.c  |  2 +-
 drivers/usb/host/xhci-dbgcap.c   |  2 +-
 drivers/usb/phy/phy-tahvo.c  |  2 +-
 drivers/video/fbdev/auo_k190x.c  |  4 ++--
 drivers/video/fbdev/w100fb.c |  4 ++--
 lib/test_firmware.c  | 14 +---
 lib/test_kmod.c  | 14 +---
 sound/soc/omap/mcbsp.c   |  4 ++--
 15 files changed, 49 insertions(+), 80 deletions(-)

diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index 4d5b65e527b5..4b6e0397f66d 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -404,8 +404,7 @@ static ssize_t dispatching_store(struct device *dev,
put_online_cpus();
return rc ? rc : count;
 }
-static DEVICE_ATTR(dispatching, 0644, dispatching_show,
-dispatching_store);
+static DEVICE_ATTR_RW(dispatching);
 
 static ssize_t cpu_polarization_show(struct device *dev,
 struct device_attribute *attr, char *buf)
diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c
index 825867c53853..af5024f0fb5a 100644
--- a/arch/tile/kernel/sysfs.c
+++ b/arch/tile/kernel/sysfs.c
@@ -184,7 +184,7 @@ static ssize_t hv_stats_store(struct device *dev,
return n < 0 ? n : count;
 }
 
-static DEVICE_ATTR(hv_stats, 0644, hv_stats_show, hv_stats_store);
+static DEVICE_ATTR_RW(hv_stats);
 
 static int hv_stats_device_add(struct device *dev, struct subsys_interface 
*sif)
 {
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
b/drivers/gpu/drm/i915/i915_sysfs.c
index c74a20b80182..1d0ab8ff5915 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -447,9 +447,9 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
 
 static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show, NULL);
 static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_boost_freq_mhz, S_IRUGO | S_IWUSR, 
gt_boost_freq_mhz_show, gt_boost_freq_mhz_store);
-static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, 
gt_max_freq_mhz_store);
-static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, 
gt_min_freq_mhz_store);
+static DEVICE_ATTR_RW(gt_boost_freq_mhz);
+static DEVICE_ATTR_RW(gt_max_freq_mhz);
+static DEVICE_ATTR_RW(gt_min_freq_mhz);
 
 static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show, NULL);
 
diff --git a/drivers/platform/x86/compal-laptop.c 
b/drivers/platform/x86/compal-laptop.c
index 6bcb750e1865..4f9bc72f0584 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -679,18 +679,12 @@ static int bat_writeable_property(struct power_supply 
*psy,
 /* == */
 /* Driver Globals */
 /* == */
-static DEVICE_ATTR(wake_up_pme,
-   0644, wake_up_pme_show, wake_up_pme_store);
-static DEVICE_ATTR(wake_up_modem,
-   0644, wake_up_modem_show,   wake_up_modem_store);
-static DEVICE_ATTR(wake_up_lan,
-   0644, wake_up_lan_show, wake_up_lan_store);
-static DEVICE_ATTR(wake_up_wlan,
-   0644, wake_up_wlan_show,wake_up_wlan_store);
-static DEVICE_ATTR(wake_up_key,
-   0644, wake_up_key_show, wake_up_key_store);
-static DEVICE_ATTR(wake_up_mouse,
-   0644, wake_up_mouse_show,   wake_up_mouse_store);
+static DEVICE_ATTR_RW(wake_up_pme);
+static DEVICE_ATTR_RW(wake_up_modem);
+static DEVICE_ATTR_RW(wake_up_lan);
+static DEVICE_ATTR_RW(wake_up_wlan);
+static DEVICE_ATTR_RW(wake_up_key);
+static DEVICE_ATTR_RW(wake_up_mouse);
 
 static DEVICE_ATTR(fan1_input,  S_IRUGO, fan_show,  NULL);
 static DEVICE_ATTR(temp1_input, S_IRUGO, temp_cpu,  NULL);
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index 75a245f38e2e..6eefb67b31f3 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -600,7 +600,7 @@ static ssize_t vpm_show(struct device *dev, struct 
device_attribute *attr,
 static DEVICE_ATTR(devtype, 0444, devtype_show, NULL);
 static DEVICE_ATTR(cutype, 0444, cutype_show, NULL);
 static DEVICE_ATTR(modalias, 0444, moda

[-next PATCH 3/4] treewide: Use DEVICE_ATTR_RO

[Joe Perches]

Convert DEVICE_ATTR uses to DEVICE_ATTR_RO where possible.

Done with perl script:

$ git grep -w --name-only DEVICE_ATTR | \
  xargs perl -i -e 'local $/; while (<>) { 
s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(?:\s*S_IRUGO\s*|\s*0444\s*)\)?\s*,\s*\1_show\s*,\s*NULL\s*\)/DEVICE_ATTR_RO(\1)/g;
 print;}'

Signed-off-by: Joe Perches <j...@perches.com>
---
 arch/arm/mach-pxa/sharpsl_pm.c   |  4 ++--
 arch/sh/drivers/push-switch.c|  2 +-
 arch/tile/kernel/sysfs.c | 10 +-
 drivers/acpi/device_sysfs.c  |  6 +++---
 drivers/char/ipmi/ipmi_msghandler.c  | 17 -
 drivers/gpu/drm/i915/i915_sysfs.c|  6 +++---
 drivers/nvme/host/core.c | 10 +-
 drivers/s390/cio/css.c   |  8 
 drivers/s390/cio/device.c|  8 
 drivers/s390/crypto/ap_card.c|  2 +-
 drivers/scsi/hpsa.c  | 10 +-
 drivers/scsi/lpfc/lpfc_attr.c| 18 --
 drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c |  8 
 drivers/thermal/thermal_sysfs.c  |  6 +++---
 sound/soc/soc-core.c |  2 +-
 sound/soc/soc-dapm.c |  2 +-
 16 files changed, 58 insertions(+), 61 deletions(-)

diff --git a/arch/arm/mach-pxa/sharpsl_pm.c b/arch/arm/mach-pxa/sharpsl_pm.c
index 398ba9ba2632..ef9fd9b759cb 100644
--- a/arch/arm/mach-pxa/sharpsl_pm.c
+++ b/arch/arm/mach-pxa/sharpsl_pm.c
@@ -802,8 +802,8 @@ static ssize_t battery_voltage_show(struct device *dev, 
struct device_attribute
return sprintf(buf, "%d\n", sharpsl_pm.battstat.mainbat_voltage);
 }
 
-static DEVICE_ATTR(battery_percentage, 0444, battery_percentage_show, NULL);
-static DEVICE_ATTR(battery_voltage, 0444, battery_voltage_show, NULL);
+static DEVICE_ATTR_RO(battery_percentage);
+static DEVICE_ATTR_RO(battery_voltage);
 
 extern void (*apm_get_power_status)(struct apm_power_info *);
 
diff --git a/arch/sh/drivers/push-switch.c b/arch/sh/drivers/push-switch.c
index a17181160233..762bc5619910 100644
--- a/arch/sh/drivers/push-switch.c
+++ b/arch/sh/drivers/push-switch.c
@@ -24,7 +24,7 @@ static ssize_t switch_show(struct device *dev,
struct push_switch_platform_info *psw_info = dev->platform_data;
return sprintf(buf, "%s\n", psw_info->name);
 }
-static DEVICE_ATTR(switch, S_IRUGO, switch_show, NULL);
+static DEVICE_ATTR_RO(switch);
 
 static void switch_timer(struct timer_list *t)
 {
diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c
index af5024f0fb5a..b09456a3d77a 100644
--- a/arch/tile/kernel/sysfs.c
+++ b/arch/tile/kernel/sysfs.c
@@ -38,7 +38,7 @@ static ssize_t chip_width_show(struct device *dev,
 {
return sprintf(page, "%u\n", smp_width);
 }
-static DEVICE_ATTR(chip_width, 0444, chip_width_show, NULL);
+static DEVICE_ATTR_RO(chip_width);
 
 static ssize_t chip_height_show(struct device *dev,
struct device_attribute *attr,
@@ -46,7 +46,7 @@ static ssize_t chip_height_show(struct device *dev,
 {
return sprintf(page, "%u\n", smp_height);
 }
-static DEVICE_ATTR(chip_height, 0444, chip_height_show, NULL);
+static DEVICE_ATTR_RO(chip_height);
 
 static ssize_t chip_serial_show(struct device *dev,
struct device_attribute *attr,
@@ -54,7 +54,7 @@ static ssize_t chip_serial_show(struct device *dev,
 {
return get_hv_confstr(page, HV_CONFSTR_CHIP_SERIAL_NUM);
 }
-static DEVICE_ATTR(chip_serial, 0444, chip_serial_show, NULL);
+static DEVICE_ATTR_RO(chip_serial);
 
 static ssize_t chip_revision_show(struct device *dev,
  struct device_attribute *attr,
@@ -62,7 +62,7 @@ static ssize_t chip_revision_show(struct device *dev,
 {
return get_hv_confstr(page, HV_CONFSTR_CHIP_REV);
 }
-static DEVICE_ATTR(chip_revision, 0444, chip_revision_show, NULL);
+static DEVICE_ATTR_RO(chip_revision);
 
 
 static ssize_t type_show(struct device *dev,
@@ -71,7 +71,7 @@ static ssize_t type_show(struct device *dev,
 {
return sprintf(page, "tilera\n");
 }
-static DEVICE_ATTR(type, 0444, type_show, NULL);
+static DEVICE_ATTR_RO(type);
 
 #define HV_CONF_ATTR(name, conf)   \
static ssize_t name ## _show(struct device *dev,\
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index a041689e5701..545e91420cde 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -357,7 +357,7 @@ static ssize_t real_power_state_show(struct device *dev,
return sprintf(buf, "%s\n", acpi_power_state_string(state));
 }
 
-static DEVICE_ATTR(real_power_state, 0444, real_power_

[trivial PATCH] treewide: Align function definition open/close braces

[Joe Perches]

Some functions definitions have either the initial open brace and/or
the closing brace outside of column 1.

Move those braces to column 1.

This allows various function analyzers like gnu complexity to work
properly for these modified functions.

Miscellanea:

o Remove extra trailing ; and blank line from xfs_agf_verify

Signed-off-by: Joe Perches <j...@perches.com>
---
git diff -w shows no difference other than the above 'Miscellanea'

(this is against -next, but it applies against Linus' tree
 with a couple offsets)

 arch/x86/include/asm/atomic64_32.h   |  2 +-
 drivers/acpi/custom_method.c |  2 +-
 drivers/acpi/fan.c   |  2 +-
 drivers/gpu/drm/amd/display/dc/core/dc.c |  2 +-
 drivers/media/i2c/msp3400-kthreads.c |  2 +-
 drivers/message/fusion/mptsas.c  |  2 +-
 drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c |  2 +-
 drivers/net/wireless/ath/ath9k/xmit.c|  2 +-
 drivers/platform/x86/eeepc-laptop.c  |  2 +-
 drivers/rtc/rtc-ab-b5ze-s3.c |  2 +-
 drivers/scsi/dpt_i2o.c   |  2 +-
 drivers/scsi/sym53c8xx_2/sym_glue.c  |  2 +-
 fs/locks.c   |  2 +-
 fs/ocfs2/stack_user.c|  2 +-
 fs/xfs/libxfs/xfs_alloc.c|  5 ++---
 fs/xfs/xfs_export.c  |  2 +-
 kernel/audit.c   |  6 +++---
 kernel/trace/trace_printk.c  |  4 ++--
 lib/raid6/sse2.c | 14 +++---
 sound/soc/fsl/fsl_dma.c  |  2 +-
 20 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/atomic64_32.h 
b/arch/x86/include/asm/atomic64_32.h
index 97c46b8169b7..d4d4883080fa 100644
--- a/arch/x86/include/asm/atomic64_32.h
+++ b/arch/x86/include/asm/atomic64_32.h
@@ -122,7 +122,7 @@ static inline long long atomic64_read(const atomic64_t *v)
long long r;
alternative_atomic64(read, "=" (r), "c" (v) : "memory");
return r;
- }
+}
 
 /**
  * atomic64_add_return - add and return
diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index c68e72414a67..e967c1173ba3 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -94,7 +94,7 @@ static void __exit acpi_custom_method_exit(void)
 {
if (cm_dentry)
debugfs_remove(cm_dentry);
- }
+}
 
 module_init(acpi_custom_method_init);
 module_exit(acpi_custom_method_exit);
diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 6cf4988206f2..3563103590c6 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -219,7 +219,7 @@ fan_set_cur_state(struct thermal_cooling_device *cdev, 
unsigned long state)
return fan_set_state_acpi4(device, state);
else
return fan_set_state(device, state);
- }
+}
 
 static const struct thermal_cooling_device_ops fan_cooling_ops = {
.get_max_state = fan_get_max_state,
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index d1488d5ee028..1e0d1e7c5324 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -461,7 +461,7 @@ static void disable_dangling_plane(struct dc *dc, struct 
dc_state *context)
  
**/
 
 struct dc *dc_create(const struct dc_init_data *init_params)
- {
+{
struct dc *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
unsigned int full_pipe_count;
 
diff --git a/drivers/media/i2c/msp3400-kthreads.c 
b/drivers/media/i2c/msp3400-kthreads.c
index 4dd01e9f553b..dc6cb8d475b3 100644
--- a/drivers/media/i2c/msp3400-kthreads.c
+++ b/drivers/media/i2c/msp3400-kthreads.c
@@ -885,7 +885,7 @@ static int msp34xxg_modus(struct i2c_client *client)
 }
 
 static void msp34xxg_set_source(struct i2c_client *client, u16 reg, int in)
- {
+{
struct msp_state *state = to_state(i2c_get_clientdata(client));
int source, matrix;
 
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 345f6035599e..69a62d23514b 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -2968,7 +2968,7 @@ mptsas_exp_repmanufacture_info(MPT_ADAPTER *ioc,
mutex_unlock(>sas_mgmt.mutex);
 out:
return ret;
- }
+}
 
 static void
 mptsas_parse_device_info(struct sas_identify *identify,
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c 
b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
index 3dd973475125..0ea141ece19e 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
@@ -603,7 +603,7 @@ static s

Re: [PATCH 01/10] arch:powerpc: return -ENOMEM on failed allocation

[Joe Perches]

On Wed, 2017-09-13 at 13:02 +0530, Allen Pais wrote:
> Signed-off-by: Allen Pais 

I think the changelog for this series of conversions
should show that you've validated the change by
inspecting the return call chain at each modified line.

Also, it seems you've cc'd the same mailing lists for
all of the patches modified by this series.

It would be better to individually address each patch
in the series only cc'ing the appropriate maintainers
and mailing lists.

A cover letter would be good too.

> ---
>  arch/powerpc/platforms/cell/spider-pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/cell/spider-pci.c 
> b/arch/powerpc/platforms/cell/spider-pci.c
> index d1e61e2..82aa3f7 100644
> --- a/arch/powerpc/platforms/cell/spider-pci.c
> +++ b/arch/powerpc/platforms/cell/spider-pci.c
> @@ -106,7 +106,7 @@ static int __init spiderpci_pci_setup_chip(struct 
> pci_controller *phb,
>   dummy_page_va = kmalloc(PAGE_SIZE, GFP_KERNEL);
>   if (!dummy_page_va) {
>   pr_err("SPIDERPCI-IOWA:Alloc dummy_page_va failed.\n");
> - return -1;
> + return -ENOMEM;
>   }
>  
>   dummy_page_da = dma_map_single(phb->parent, dummy_page_va,
> @@ -137,7 +137,7 @@ int __init spiderpci_iowa_init(struct iowa_bus *bus, void 
> *data)
>   if (!priv) {
>   pr_err("SPIDERPCI-IOWA:"
>  "Can't allocate struct spiderpci_iowa_private");
> - return -1;
> + return -ENOMEM;
>   }
>  
>   if (of_address_to_resource(np, 0, )) {

Re: [PATCH] scsi: remove memset before memcpy

[Joe Perches]

On Wed, 2017-08-30 at 00:19 +0530, Himanshu Jha wrote:
> drivers/scsi/megaraid/megaraid_sas_fusion.c

I don't know if you did this with coccinelle.

If so, it would be good to show the tool and script
in the commit message.

If not, the input script is pretty simple.

$ cat memset_before_memcpy.cocci
@@
expression e1;
expression e2;
expression e3;
@@

-   memset(e1, 0, e3);
memcpy(e1, e2, e3);
$

Adding a test to make sure e1 or e3 isn't
modified before any other code uses them
by doing

$ cat memset_before_memcpy_2.cocci
@@
expression e1;
expression e2;
expressi
on e3;
@@

-   memset(e1, 0, e3);
... when != \( e1 \| e3 \)
memcpy(e1, e2, e3);
$

finds more cases but there may be a
false positive if e1 is a passed
function argument and if the operation
isn't effectively atomic like below:


--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -2556,7 +2556,6 @@ void br_multicast_get_stats(const struct
    struct br_mcast_stats tdst;
    int i;
 
-   memset(dest, 0, sizeof(*dest));
    if (p)
    stats = p->mcast_stats;
    else



where the memcpy is the last line of the function.

Re: [PATCH] lpfc: nvmet_fc: fix format string

[Joe Perches]

On Sat, 2017-05-20 at 21:10 +0200, Arnd Bergmann wrote:
> On Sat, May 20, 2017 at 12:28 PM, Joe Perches <j...@perches.com> wrote:
> > On Fri, 2017-05-19 at 10:04 +0200, Arnd Bergmann wrote:
> > > The lpfc_nvmeio_data() tracing helper always takes a format string and
> > > three additional arguments.
> > 
> > No it doesn't.  It takes a format and arguments.
> > 
> > I don't disagree with the patch, just the characterization
> > of the lpfc_mvmeio_data call in the commit message.
> 
> I think my description is correct, it's just not obvious from
> reading the code until you also look at the lpfc_debugfs_nvme_trc
> prototype:

OK, but more that's a mismatch between a function and its
arguments and a different called function within it.

Re: [PATCH 1/2] scsi: nsp32: add __printf attribute to logging functions

[Joe Perches]

On Sat, 2017-05-20 at 13:16 +0200, Nicolas Iooss wrote:
> nsp32_message() and nsp32_dmessage() use printf format strings in order
> to format a message. Adding __printf attributes helps to detect errors
> in such format strings at build time, like:
> 
> drivers/scsi/nsp32.c:3314:23: error: format '%ld' expects argument
> of type 'long int', but argument 6 has type 'pm_message_t {aka
> struct pm_message}' [-Werror=format=]
>   nsp32_msg(KERN_INFO,
>   "pci-suspend: pdev=0x%p, state=%ld, slot=%s, host=0x%p",
>   pdev, state, pci_name(pdev), host);
> 
> Fix all format string errors which were reported by gcc.
[]
> diff --git a/drivers/scsi/nsp32.c b/drivers/scsi/nsp32.c
[]
> @@ -321,7 +323,8 @@ static struct scsi_host_template nsp32_template = {
>  
>  #define NSP32_DEBUG_BUF_LEN  100
>  
> -static void nsp32_message(const char *func, int line, char *type, char *fmt, 
> ...)
> +static __printf(4, 5)
> +void nsp32_message(const char *func, int line, char *type, const char *fmt, 
> ...)
>  {
>   va_list args;
>   char buf[NSP32_DEBUG_BUF_LEN];

These could also use vsprintf extension %pV instead of
vsnprintf to a
temporary buffer and then using "%s, "

etc...

Does anyone actually have or use these cards any longer?

Re: [PATCH] lpfc: nvmet_fc: fix format string

[Joe Perches]

On Fri, 2017-05-19 at 10:04 +0200, Arnd Bergmann wrote:
> The lpfc_nvmeio_data() tracing helper always takes a format string and
> three additional arguments.

No it doesn't.  It takes a format and arguments.

I don't disagree with the patch, just the characterization
of the lpfc_mvmeio_data call in the commit message.

> The latest caller has a format string with
> only two integer arguments, causing this harmless warning:
> 
> drivers/scsi/lpfc/lpfc_nvmet.c: In function 'lpfc_nvmet_xmt_fcp_release':
> drivers/scsi/lpfc/lpfc_nvmet.c:802:25: error: too many arguments for format 
> [-Werror=format-extra-args]
>   lpfc_nvmeio_data(phba, "NVMET FCP FREE: xri x%x ste %d\n", ctxp->oxid,
> 
> We could add a dummy argument here, but it seems reasonable to print
> the 'abort' flag as the third argument.

[PATCH] scsi: fas216: Add __printf validation, fix fallout

[Joe Perches]

__printf makes the compiler check format and arguments.

Fix fallout.

Miscellanea:

o Convert formats to const char *
o Use vsprintf extension %pV instead of a static buffer.
o Add newline to logging and remove now unnecessary printk("\n")
o Use pr_cont where appropriate

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/arm/fas216.c | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
index 24388795ee9a..112bec886192 100644
--- a/drivers/scsi/arm/fas216.c
+++ b/drivers/scsi/arm/fas216.c
@@ -289,17 +289,20 @@ static char fas216_target(FAS216_Info *info)
return 'H';
 }
 
-static void
-fas216_do_log(FAS216_Info *info, char target, char *fmt, va_list ap)
+__printf(3, 0) static void
+fas216_do_log(FAS216_Info *info, char target, const char *fmt, va_list ap)
 {
-   static char buf[1024];
+   struct va_format vaf;
+
+   vaf.fmt = fmt;
+   vaf.va = 
 
-   vsnprintf(buf, sizeof(buf), fmt, ap);
-   printk("scsi%d.%c: %s", info->host->host_no, target, buf);
+   printk("scsi%d.%c: %pV\n", info->host->host_no, target, );
 }
 
+__printf(4, 5)
 static void fas216_log_command(FAS216_Info *info, int level,
-  struct scsi_cmnd *SCpnt, char *fmt, ...)
+  struct scsi_cmnd *SCpnt, const char *fmt, ...)
 {
va_list args;
 
@@ -313,8 +316,9 @@ static void fas216_log_command(FAS216_Info *info, int level,
scsi_print_command(SCpnt);
 }
 
-static void
-fas216_log_target(FAS216_Info *info, int level, int target, char *fmt, ...)
+__printf(4, 5) static void
+fas216_log_target(FAS216_Info *info, int level, int target,
+ const char *fmt, ...)
 {
va_list args;
 
@@ -329,11 +333,10 @@ fas216_log_target(FAS216_Info *info, int level, int 
target, char *fmt, ...)
va_start(args, fmt);
fas216_do_log(info, target, fmt, args);
va_end(args);
-
-   printk("\n");
 }
 
-static void fas216_log(FAS216_Info *info, int level, char *fmt, ...)
+__printf(3, 4)
+static void fas216_log(FAS216_Info *info, int level, const char *fmt, ...)
 {
va_list args;
 
@@ -343,8 +346,6 @@ static void fas216_log(FAS216_Info *info, int level, char 
*fmt, ...)
va_start(args, fmt);
fas216_do_log(info, fas216_target(info), fmt, args);
va_end(args);
-
-   printk("\n");
 }
 
 #define PH_SIZE32
@@ -431,7 +432,7 @@ fas216_get_last_msg(FAS216_Info *info, int pos)
}
 
fas216_log(info, LOG_MESSAGES,
-   "Message: %04x found at position %02x\n", packed_msg, pos);
+  "Message: %04x found at position %02x", packed_msg, pos);
 
return packed_msg;
 }
@@ -725,7 +726,7 @@ static void fas216_cleanuptransfer(FAS216_Info *info)
fifo = fas216_readb(info, REG_CFIS) & CFIS_CF;
 
fas216_log(info, LOG_BUFFER, "cleaning up from previous "
-  "transfer: length 0x%06x, residual 0x%x, fifo %d",
+  "transfer: length 0x%06lx, residual 0x%lx, fifo %ld",
   total, residual, fifo);
 
/*
@@ -1144,8 +1145,8 @@ static void fas216_parse_message(FAS216_Info *info, 
unsigned char *message, int
fas216_log(info, 0, "unrecognised message, rejecting");
printk("scsi%d.%c: message was", info->host->host_no, 
fas216_target(info));
for (i = 0; i < msglen; i++)
-   printk("%s%02X", i & 31 ? " " : "\n  ", message[i]);
-   printk("\n");
+   pr_cont("%s%02X", i & 31 ? " " : "\n  ", message[i]);
+   pr_cont("\n");
 
/*
 * Something strange seems to be happening here -
@@ -1582,7 +1583,7 @@ static void fas216_funcdone_intr(FAS216_Info *info, 
unsigned int stat, unsigned
default:
fas216_log(info, 0, "internal phase %s for function done?"
"  What do I do with this?",
-   fas216_target(info), fas216_drv_phase(info));
+   fas216_drv_phase(info));
}
 }
 
@@ -1642,7 +1643,7 @@ irqreturn_t fas216_intr(FAS216_Info *info)
fas216_bus_reset(info);
scsi_report_bus_reset(info->host, 0);
} else if (inst & INST_ILLEGALCMD) {
-   fas216_log(info, LOG_ERROR, "illegal command given\n");
+   fas216_log(info, LOG_ERROR, "illegal command given");
fas216_dumpstate(info);
print_debug_list();
} else if (inst & INST_DISCONNECT)
-- 
2.10.0.rc2.1.g053435c

Re: Checking error messages for failed memory allocations

[Joe Perches]

On Wed, 2017-04-26 at 20:50 +0200, SF Markus Elfring wrote:
> > Basically most everything that has a gfp_t argument does a
> > dump_stack() on OOM unless __GFP_NOWARN is specified by that gfp_t.
> 
> How do you think about to improve any programming interface documentation
> around such a function property?

Feel free to submit documentation patches.

> Are there any special checks needed for function implementations
> which can pass the flag “__GFP_NOWARN”?

No.

Re: [PATCH 2/3] scsi: ufs: Delete an error message for a failed memory allocation in ufshcd_memory_alloc()

[Joe Perches]

On Wed, 2017-04-26 at 10:57 -0700, Subhash Jadavani wrote:
> PS: ufshcd_memory_alloc() also does some DMA coherent memory allocation 
> (via dmam_alloc_coherent() APIs) and tries to print out the message on 
> allocation failure. Although i don't know "out of memory" messages will 
> be printed out by dmam_alloc_coherent() APIs or not. If it does print it 
> out then we might want to remove our local memory allocation failure log 
> messages.

Basically most everything that has a gfp_t argument does a
dump_stack() on OOM unless __GFP_NOWARN is specified by that gfp_t.

Re: [PATCH] scsi: qedf: Fix defective logging format and argument mismatches

[Joe Perches]

On Mon, 2017-03-06 at 10:17 -0800, Joe Perches wrote:
> On Tue, 2017-03-07 at 02:03 +0800, kbuild test robot wrote:
> > Hi Joe,
> 
> Hi again Fengguang's robot.

Rehi.  OK, it is a new message.  Patch sent.

[PATCH] scsi: qedf: Use vsprintf extension %pad

[Joe Perches]

Using %llx for a dma_addr_t can lead to format/argument mismatches.
Use %pad and the address of the dma_addr_t instead.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/qedf/qedf_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index d9d7a86b5f8b..8e2a160490e6 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -2456,8 +2456,8 @@ static int qedf_alloc_bdq(struct qedf_ctx *qedf)
}
 
QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_DISC,
-   "BDQ PBL addr=0x%p dma=0x%llx.\n", qedf->bdq_pbl,
-   qedf->bdq_pbl_dma);
+ "BDQ PBL addr=0x%p dma=%pad\n",
+ qedf->bdq_pbl, >bdq_pbl_dma);
 
/*
 * Populate BDQ PBL with physical and virtual address of individual
-- 
2.10.0.rc2.1.g053435c

Re: [PATCH] scsi: qedf: Fix defective logging format and argument mismatches

[Joe Perches]

On Tue, 2017-03-07 at 02:03 +0800, kbuild test robot wrote:
> Hi Joe,

Hi again Fengguang's robot.

> [auto build test WARNING on scsi/for-next]
> [also build test WARNING on v4.11-rc1 next-20170306]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Joe-Perches/scsi-qedf-Fix-defective-logging-format-and-argument-mismatches/20170307-005400
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
> config: parisc-allmodconfig (attached as .config)
> compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705

Not new messages.

[PATCH] scsi: qedf: Fix defective logging format and argument mismatches

[Joe Perches]

Add __printf compiler verification of format and arguments.
Fix fallout.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/qedf/qedf_dbg.h | 13 -
 drivers/scsi/qedf/qedf_fip.c |  2 +-
 drivers/scsi/qedf/qedf_io.c  |  4 ++--
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_dbg.h b/drivers/scsi/qedf/qedf_dbg.h
index 23bd70628a2f..7d173f48a81e 100644
--- a/drivers/scsi/qedf/qedf_dbg.h
+++ b/drivers/scsi/qedf/qedf_dbg.h
@@ -81,14 +81,17 @@ struct qedf_dbg_ctx {
 #define QEDF_INFO(pdev, level, fmt, ...)   \
qedf_dbg_info(pdev, __func__, __LINE__, level, fmt, \
  ## __VA_ARGS__)
-
-extern void qedf_dbg_err(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
+__printf(4, 5)
+void qedf_dbg_err(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
  const char *fmt, ...);
-extern void qedf_dbg_warn(struct qedf_dbg_ctx *qedf, const char *func, u32 
line,
+__printf(4, 5)
+void qedf_dbg_warn(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
   const char *, ...);
-extern void qedf_dbg_notice(struct qedf_dbg_ctx *qedf, const char *func,
+__printf(4, 5)
+void qedf_dbg_notice(struct qedf_dbg_ctx *qedf, const char *func,
u32 line, const char *, ...);
-extern void qedf_dbg_info(struct qedf_dbg_ctx *qedf, const char *func, u32 
line,
+__printf(5, 6)
+void qedf_dbg_info(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
  u32 info, const char *fmt, ...);
 
 /* GRC Dump related defines */
diff --git a/drivers/scsi/qedf/qedf_fip.c b/drivers/scsi/qedf/qedf_fip.c
index 868d423380d1..ed58b9104f58 100644
--- a/drivers/scsi/qedf/qedf_fip.c
+++ b/drivers/scsi/qedf/qedf_fip.c
@@ -203,7 +203,7 @@ void qedf_fip_recv(struct qedf_ctx *qedf, struct sk_buff 
*skb)
case FIP_DT_MAC:
mp = (struct fip_mac_desc *)desc;
QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_LL2,
-   "fd_mac=%pM.\n", __func__, mp->fd_mac);
+   "fd_mac=%pM\n", mp->fd_mac);
ether_addr_copy(cvl_mac, mp->fd_mac);
break;
case FIP_DT_NAME:
diff --git a/drivers/scsi/qedf/qedf_io.c b/drivers/scsi/qedf/qedf_io.c
index ee0dcf9d3aba..46debe5034af 100644
--- a/drivers/scsi/qedf/qedf_io.c
+++ b/drivers/scsi/qedf/qedf_io.c
@@ -1342,7 +1342,7 @@ void qedf_scsi_completion(struct qedf_ctx *qedf, struct 
fcoe_cqe *cqe,
} else {
refcount = kref_read(_req->refcount);
QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_IO,
-   "%d:0:%d:%d xid=0x%0x op=0x%02x "
+   "%d:0:%d:%lld xid=0x%0x op=0x%02x "
"lba=%02x%02x%02x%02x cdb_status=%d "
"fcp_resid=0x%x refcount=%d.\n",
qedf->lport->host->host_no, sc_cmd->device->id,
@@ -1426,7 +1426,7 @@ void qedf_scsi_done(struct qedf_ctx *qedf, struct 
qedf_ioreq *io_req,
 
sc_cmd->result = result << 16;
refcount = kref_read(_req->refcount);
-   QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_IO, "%d:0:%d:%d: Completing "
+   QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_IO, "%d:0:%d:%lld: Completing "
"sc_cmd=%p result=0x%08x op=0x%02x lba=0x%02x%02x%02x%02x, "
"allowed=%d retries=%d refcount=%d.\n",
qedf->lport->host->host_no, sc_cmd->device->id,
-- 
2.10.0.rc2.1.g053435c

[PATCH] qla2xxx: Fix ql_dump_buffer

[Joe Perches]

Recent printk changes for KERN_CONT cause this logging to be
defectively emitted on multiple lines.  Fix it.

Also reduces object size a trivial amount.

$ size drivers/scsi/qla2xxx/qla_dbg.o*
   textdata bss dec hex filename
  39125   0   0   3912598d5 drivers/scsi/qla2xxx/qla_dbg.o.new
  39164   0   0   3916498fc drivers/scsi/qla2xxx/qla_dbg.o.old

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/qla2xxx/qla_dbg.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 21d9fb7fc887..51b4179469d1 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -2707,13 +2707,9 @@ ql_dump_buffer(uint32_t level, scsi_qla_host_t *vha, 
int32_t id,
"%-+5d  0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F\n", size);
ql_dbg(level, vha, id,
"- ---\n");
-   for (cnt = 0; cnt < size; cnt++, buf++) {
-   if (cnt % 16 == 0)
-   ql_dbg(level, vha, id, "%04x:", cnt & ~0xFU);
-   printk(" %02x", *buf);
-   if (cnt % 16 == 15)
-   printk("\n");
+   for (cnt = 0; cnt < size; cnt += 16) {
+   ql_dbg(level, vha, id, "%04x: ", cnt);
+   print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16, 1,
+  buf + cnt, min(16U, size - cnt), false);
}
-   if (cnt % 16 != 0)
-   printk("\n");
 }
-- 
2.10.0.rc2.1.g053435c

Re: [PATCH v4 00/19] Replace PCI pool by DMA pool API

[Joe Perches]

On Wed, 2017-03-01 at 16:55 +0100, Romain Perier wrote:
> support to warn about this old API in checkpath.pl

checkpatch

This part isn't true anymore, but it seems sensible enough, thanks.

Re: [PATCH v3 20/20] checkpatch: warn for use of old PCI pool API

[Joe Perches]

On Mon, 2017-02-27 at 13:52 +0100, Romain Perier wrote:

> > I also wonder if you've in fact converted all of the
> > pci_pool struct and function uses why a new checkpatch
> > test is needed at all.
> 
> That's just to avoid futures mistakes/uses.

When all instances and macro definitions are removed
the check is pointless as any newly submitted patch
will not compile.

Re: [PATCH v3 20/20] checkpatch: warn for use of old PCI pool API

[Joe Perches]

On Mon, 2017-02-27 at 13:26 +0100, Romain Perier wrote:
> Hello,
> 
> 
> Le 27/02/2017 à 12:22, Peter Senna Tschudin a écrit :
> > On Sun, Feb 26, 2017 at 08:24:25PM +0100, Romain Perier wrote:
> > > pci_pool_*() functions should be replaced by the corresponding functions
> > > in the DMA pool API. This adds support to check for use of these pci
> > > functions and display a warning when it is the case.
> > > 
> > 
> > I guess Joe Perches did sent some comments for this one, did you address
> > them?
> 
> See the changelog of 00/20 (for v2). I have already integrated his
> comments :)

Not quite.  You need to add blank lines before and after
the new test you added.

I also wonder if you've in fact converted all of the
pci_pool struct and function uses why a new checkpatch
test is needed at all.

Also, it seems none of these patches have reached lkml.
Are you sending the patch series with MIME/html parts?  

> > Reviewed-by: Peter Senna Tschudin <peter.se...@collabora.com>
> > > Signed-off-by: Romain Perier <romain.per...@collabora.com>
> > > ---
> > >  scripts/checkpatch.pl | 9 -
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index baa3c7b..f2c775c 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -6064,7 +6064,14 @@ sub process {
> > >   WARN("USE_DEVICE_INITCALL",
> > >"please use device_initcall() or more appropriate 
> > > function instead of __initcall() (see include/linux/init.h)\n" . 
> > > $herecurr);
> > >   }
> > > -
> > > +# check for old PCI api pci_pool_*(), use dma_pool_*() instead
> > > + if ($line =~ 
> > > /\bpci_pool(?:_(?:create|destroy|alloc|zalloc|free)|)\b/) {
> > > + if (WARN("USE_DMA_POOL",
> > > +  "please use the dma pool api or more 
> > > appropriate function instead of the old pci pool api\n" . $herecurr) &&
> > > + $fix) {
> > > + while ($fixed[$fixlinenr] =~ 
> > > s/\bpci_pool(_(?:create|destroy|alloc|zalloc|free)|)\b/dma_pool$1/) {}
> > > + }
> > > + }
> > >  # check for various structs that are normally const (ops, kgdb, 
> > > device_tree)
> > >   if ($line !~ /\bconst\b/ &&
> > >   $line =~ /\bstruct\s+($const_structs)\b/) {
> > > -- 
> > > 2.9.3

Re: [PATCH v3 20/20] checkpatch: warn for use of old PCI pool API

[Joe Perches]

On Mon, 2017-02-27 at 12:22 +0100, Peter Senna Tschudin wrote:
> On Sun, Feb 26, 2017 at 08:24:25PM +0100, Romain Perier wrote:
> > pci_pool_*() functions should be replaced by the corresponding functions
> > in the DMA pool API. This adds support to check for use of these pci
> > functions and display a warning when it is the case.
> > 
> 
> I guess Joe Perches did sent some comments for this one, did you address
> them?


> Reviewed-by: Peter Senna Tschudin <peter.se...@collabora.com>
> > Signed-off-by: Romain Perier <romain.per...@collabora.com>
> > ---
> >  scripts/checkpatch.pl | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index baa3c7b..f2c775c 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -6064,7 +6064,14 @@ sub process {
> > WARN("USE_DEVICE_INITCALL",
> >  "please use device_initcall() or more appropriate 
> > function instead of __initcall() (see include/linux/init.h)\n" . $herecurr);
> > }
> > -
> > +# check for old PCI api pci_pool_*(), use dma_pool_*() instead
> > +   if ($line =~ 
> > /\bpci_pool(?:_(?:create|destroy|alloc|zalloc|free)|)\b/) {
> > +   if (WARN("USE_DMA_POOL",
> > +"please use the dma pool api or more 
> > appropriate function instead of the old pci pool api\n" . $herecurr) &&
> > +   $fix) {
> > +   while ($fixed[$fixlinenr] =~ 
> > s/\bpci_pool(_(?:create|destroy|alloc|zalloc|free)|)\b/dma_pool$1/) {}
> > +   }
> > +   }
> >  # check for various structs that are normally const (ops, kgdb, 
> > device_tree)
> > if ($line !~ /\bconst\b/ &&
> > $line =~ /\bstruct\s+($const_structs)\b/) {
> > 

This is nearly identical to the suggestion that I
sent but this is slightly misformatted as it does
not have a leading nor a trailing blank line to
separate the test blocks.

Also, I think none of the patches have reached lkml.

Romain, are you using git-send-email to send these
patches?  Perhaps the patches you send also contain
html which are rejected by the mailing list.

Re: [PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn

[Joe Perches]

On Thu, 2017-02-23 at 17:41 +, Emil Velikov wrote:
> On 23 February 2017 at 17:18, Joe Perches <j...@perches.com> wrote:
> > On Thu, 2017-02-23 at 09:28 -0600, Rob Herring wrote:
> > > On Fri, Feb 17, 2017 at 1:11 AM, Joe Perches <j...@perches.com> wrote:
> > > > There are ~4300 uses of pr_warn and ~250 uses of the older
> > > > pr_warning in the kernel source tree.
> > > > 
> > > > Make the use of pr_warn consistent across all kernel files.
> > > > 
> > > > This excludes all files in tools/ as there is a separate
> > > > define pr_warning for that directory tree and pr_warn is
> > > > not used in tools/.
> > > > 
> > > > Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing.
> > 
> > []
> > > Where's the removal of pr_warning so we don't have more sneak in?
> > 
> > After all of these actually get applied,
> > and maybe a cycle or two later, one would
> > get sent.
> > 
> 
> By which point you'll get a few reincarnation of it. So you'll have to
> do the same exercise again :-(

Maybe to one or two files.  Not a big deal.

> I guess the question is - are you expecting to get the series merged
> all together/via one tree ?

No.  The only person that could do that effectively is Linus.

> If not, your plan is perfectly reasonable.

Re: [PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn

[Joe Perches]

On Thu, 2017-02-23 at 09:28 -0600, Rob Herring wrote:
> On Fri, Feb 17, 2017 at 1:11 AM, Joe Perches <j...@perches.com> wrote:
> > There are ~4300 uses of pr_warn and ~250 uses of the older
> > pr_warning in the kernel source tree.
> > 
> > Make the use of pr_warn consistent across all kernel files.
> > 
> > This excludes all files in tools/ as there is a separate
> > define pr_warning for that directory tree and pr_warn is
> > not used in tools/.
> > 
> > Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing.
[]
> Where's the removal of pr_warning so we don't have more sneak in?

After all of these actually get applied,
and maybe a cycle or two later, one would
get sent.

[PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn

[Joe Perches]

There are ~4300 uses of pr_warn and ~250 uses of the older
pr_warning in the kernel source tree.

Make the use of pr_warn consistent across all kernel files.

This excludes all files in tools/ as there is a separate
define pr_warning for that directory tree and pr_warn is
not used in tools/.

Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing.

Miscellanea:

o Coalesce formats and realign arguments

Some files not compiled - no cross-compilers

Joe Perches (35):
  alpha: Convert remaining uses of pr_warning to pr_warn
  ARM: ep93xx: Convert remaining uses of pr_warning to pr_warn
  arm64: Convert remaining uses of pr_warning to pr_warn
  arch/blackfin: Convert remaining uses of pr_warning to pr_warn
  ia64: Convert remaining use of pr_warning to pr_warn
  powerpc: Convert remaining uses of pr_warning to pr_warn
  sh: Convert remaining uses of pr_warning to pr_warn
  sparc: Convert remaining use of pr_warning to pr_warn
  x86: Convert remaining uses of pr_warning to pr_warn
  drivers/acpi: Convert remaining uses of pr_warning to pr_warn
  block/drbd: Convert remaining uses of pr_warning to pr_warn
  gdrom: Convert remaining uses of pr_warning to pr_warn
  drivers/char: Convert remaining use of pr_warning to pr_warn
  clocksource: Convert remaining use of pr_warning to pr_warn
  drivers/crypto: Convert remaining uses of pr_warning to pr_warn
  fmc: Convert remaining use of pr_warning to pr_warn
  drivers/gpu: Convert remaining uses of pr_warning to pr_warn
  drivers/ide: Convert remaining uses of pr_warning to pr_warn
  drivers/input: Convert remaining uses of pr_warning to pr_warn
  drivers/isdn: Convert remaining uses of pr_warning to pr_warn
  drivers/macintosh: Convert remaining uses of pr_warning to pr_warn
  drivers/media: Convert remaining use of pr_warning to pr_warn
  drivers/mfd: Convert remaining uses of pr_warning to pr_warn
  drivers/mtd: Convert remaining uses of pr_warning to pr_warn
  drivers/of: Convert remaining uses of pr_warning to pr_warn
  drivers/oprofile: Convert remaining uses of pr_warning to pr_warn
  drivers/platform: Convert remaining uses of pr_warning to pr_warn
  drivers/rapidio: Convert remaining use of pr_warning to pr_warn
  drivers/scsi: Convert remaining use of pr_warning to pr_warn
  drivers/sh: Convert remaining use of pr_warning to pr_warn
  drivers/tty: Convert remaining uses of pr_warning to pr_warn
  drivers/video: Convert remaining uses of pr_warning to pr_warn
  kernel/trace: Convert remaining uses of pr_warning to pr_warn
  lib: Convert remaining uses of pr_warning to pr_warn
  sound/soc: Convert remaining uses of pr_warning to pr_warn

 arch/alpha/kernel/perf_event.c |  4 +-
 arch/arm/mach-ep93xx/core.c|  4 +-
 arch/arm64/include/asm/syscall.h   |  8 ++--
 arch/arm64/kernel/hw_breakpoint.c  |  8 ++--
 arch/arm64/kernel/smp.c|  4 +-
 arch/blackfin/kernel/nmi.c |  2 +-
 arch/blackfin/kernel/ptrace.c  |  2 +-
 arch/blackfin/mach-bf533/boards/stamp.c|  2 +-
 arch/blackfin/mach-bf537/boards/cm_bf537e.c|  2 +-
 arch/blackfin/mach-bf537/boards/cm_bf537u.c|  2 +-
 arch/blackfin/mach-bf537/boards/stamp.c|  2 +-
 arch/blackfin/mach-bf537/boards/tcm_bf537.c|  2 +-
 arch/blackfin/mach-bf561/boards/cm_bf561.c |  2 +-
 arch/blackfin/mach-bf561/boards/ezkit.c|  2 +-
 arch/blackfin/mm/isram-driver.c|  4 +-
 arch/ia64/kernel/setup.c   |  6 +--
 arch/powerpc/kernel/pci-common.c   |  4 +-
 arch/powerpc/mm/init_64.c  |  5 +--
 arch/powerpc/mm/mem.c  |  3 +-
 arch/powerpc/platforms/512x/mpc512x_shared.c   |  4 +-
 arch/powerpc/platforms/85xx/socrates_fpga_pic.c|  7 ++--
 arch/powerpc/platforms/86xx/mpc86xx_hpcn.c |  2 +-
 arch/powerpc/platforms/pasemi/dma_lib.c|  4 +-
 arch/powerpc/platforms/powernv/opal.c  |  8 ++--
 arch/powerpc/platforms/powernv/pci-ioda.c  | 10 ++---
 arch/powerpc/platforms/ps3/device-init.c   | 14 +++
 arch/powerpc/platforms/ps3/mm.c|  4 +-
 arch/powerpc/platforms/ps3/os-area.c   |  2 +-
 arch/powerpc/platforms/pseries/iommu.c |  8 ++--
 arch/powerpc/platforms/pseries/setup.c |  4 +-
 arch/powerpc/sysdev/fsl_pci.c  |  9 ++---
 arch/powerpc/sysdev/mpic.c | 10 ++---
 arch/powerpc/sysdev/xics/icp-native.c  | 10 ++---
 arch/powerpc/sysdev/xics/ics-opal.c|  4 +-
 arch/powerpc/sysdev/xics/ics-rtas.c|  4 +-
 arch/powerpc/sysdev/xics/xics-common.c |  8 ++--
 arch/sh/boards/mach-sdk7786/nmi.c  |  2 +-
 arch/sh/drivers/pci/fixups-sdk7786.c   |  2 +-
 arch/sh/kernel/io_trapped.c

[PATCH 29/35] drivers/scsi: Convert remaining use of pr_warning to pr_warn

[Joe Perches]

To enable eventual removal of pr_warning

This makes pr_warn use consistent for drivers/scsi

Prior to this patch, there was 1 use of pr_warning and
96 uses of pr_warn in drivers/scsi

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/a3000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/a3000.c b/drivers/scsi/a3000.c
index e6375b4de79e..2be0f577abbf 100644
--- a/drivers/scsi/a3000.c
+++ b/drivers/scsi/a3000.c
@@ -38,7 +38,7 @@ static irqreturn_t a3000_intr(int irq, void *data)
spin_unlock_irqrestore(instance->host_lock, flags);
return IRQ_HANDLED;
}
-   pr_warning("Non-serviced A3000 SCSI-interrupt? ISTR = %02x\n", status);
+   pr_warn("Non-serviced A3000 SCSI-interrupt? ISTR = %02x\n", status);
return IRQ_NONE;
 }
 
-- 
2.10.0.rc2.1.g053435c

Re: [RFC 19/19] checkpatch: warn for use of old PCI pool API

[Joe Perches]

On Wed, 2017-02-08 at 19:55 +0100, Peter Senna Tschudin wrote:
> On Wed, Feb 08, 2017 at 05:34:57PM +0100, Romain Perier wrote:
> > pci_pool_*() functions should be replaced by the corresponding functions
> > in the DMA pool API. This adds support to check for use of these pci
> > functions and display a warning when it is the case.
> 
> Don't know if relevant, but did not catch the struct. Other than that
> works fine.
> 
> > 
> > Signed-off-by: Romain Perier 
> > ---
> >  scripts/checkpatch.pl | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 8e96af5..026920e 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -6058,6 +6058,11 @@ sub process {
> > WARN("USE_DEVICE_INITCALL",
> >  "please use device_initcall() or more appropriate 
> > function instead of __initcall() (see include/linux/init.h)\n" . $herecurr);
> > }
> > +# check for old PCI api pci_pool_*(), use dma_pool_*() instead
> > +   if ($line =~ /\bpci_pool_.+\(/) {
> > +   WARN("USE_PCI_POOL",
> > +"please use the dma pool api or more appropriate 
> > function instead of the old pci pool api\n" . $herecurr);
> > +   }
> >  
> >  # check for various structs that are normally const (ops, kgdb, 
> > device_tree)
> > if ($line !~ /\bconst\b/ &&
> > -- 
> > 2.9.3
> > 

Did this patch get to lkml?

Perhaps this would be more complete:

 scripts/checkpatch.pl | 9 +
 1 file changed, 9 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8e96af53611c..600f81cc1ec1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6059,6 +6059,15 @@ sub process {
     "please use device_initcall() or more appropriate 
function instead of __initcall() (see include/linux/init.h)\n" . $herecurr);
    }
 
+# check for old PCI api pci_pool_*(), use dma_pool_*() instead
+   if ($line =~ 
/\bpci_pool(?:_(?:create|destroy|alloc|zalloc|free)|)\b/) {
+   if (WARN("USE_DMA_POOL",
+    "please use the dma pool api or more 
appropriate function instead of the old pci pool api\n" . $herecurr) &&
+   $fix) {
+   while ($fixed[$fixlinenr] =~ 
s/\bpci_pool(_(?:create|destroy|alloc|zalloc|free)|)\b/dma_pool$1/) {}
+   }
+   }
+
 # check for various structs that are normally const (ops, kgdb, device_tree)
    if ($line !~ /\bconst\b/ &&
    $line =~ /\bstruct\s+($const_structs)\b/) {

Re: [PATCH] net-next: treewide use is_vlan_dev() helper function.

[Joe Perches]

On Fri, 2017-02-03 at 22:26 -0600, Parav Pandit wrote:
> This patch makes use of is_vlan_dev() function instead of flag
> comparison which is exactly done by is_vlan_dev() helper function.

Thanks.

btw:  after applying this patch, there is one left

$ git grep -E -n "&\s*IFF_802_1Q_VLAN\b" -- "*.c"
drivers/net/ethernet/chelsio/cxgb4/l2t.c:435:   if (neigh->dev->priv_flags & 
IFF_802_1Q_VLAN)

Re: [PATCH v2 1/4] mpt3sas: Added print to notify cable running at a degraded speed.

[Joe Perches]

On Fri, 2017-01-20 at 09:55 -0800, Joe Perches wrote:
> I believe MPT3SAS_FMT is unnecessary obfuscation and
> it should just be replaced by "%s: " everywhere.

Here's a trivial command that could be used one day:

$ git grep --name-only MPT3SAS_FMT -- "*.c" | \
  xargs perl -p -i -e 'local $/; while (<>) { s/\bMPT3SAS_FMT\s*"/"%s: /g; 
print; }'
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] mpt3sas: Added print to notify cable running at a degraded speed.

[Joe Perches]

On Fri, 2017-01-20 at 20:12 +0530, Chaitra P B wrote:
> Driver processes the event MPI26_EVENT_ACTIVE_CABLE_DEGRADED
> when a cable is present and is running at a degraded speed
> (below the SAS3 12 Gb/s rate). Prints added
> to inform the user that the cable is not running at
> optimal speed.
[]
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
[]
> @@ -8028,15 +8028,23 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER 
> *ioc, u8 msix_index,
>   case MPI2_EVENT_ACTIVE_CABLE_EXCEPTION:
>   ActiveCableEventData =
>   (Mpi26EventDataActiveCableExcept_t *) mpi_reply->EventData;
> - if (ActiveCableEventData->ReasonCode ==
> - MPI26_EVENT_ACTIVE_CABLE_INSUFFICIENT_POWER) {
> - pr_info(MPT3SAS_FMT "Currently an active cable with 
> ReceptacleID %d",
> + switch (ActiveCableEventData->ReasonCode) {
> + case MPI26_EVENT_ACTIVE_CABLE_INSUFFICIENT_POWER:
> + pr_info(MPT3SAS_FMT "Currently an active cable with 
> ReceptacleID %d\n",
>   ioc->name, ActiveCableEventData->ReceptacleID);
> - pr_info("cannot be powered and devices connected to 
> this active cable");
> - pr_info("will not be seen. This active cable");
> - pr_info("requires %d mW of power",
> + pr_info(" cannot be powered and devices connected 
> to\n");
> + pr_info(" this active cable will not be seen. This\n");
> + pr_info(" cable requires %d mW of power\n",

Can you please use more intelligible logging
where sentences are not broken across multiple
lines of output?  Something like:

pr_notice(MPT3SAS_FMT "Receptacle ID %d: This active 
cable requires %d mW of power\n",
  ioc->name, ActiveCableEventData->ReceptacleID,
  
ActiveCableEventData->ActiveCablePowerRequirement);
pr_notice(MPT3SAS_FMT "Receptacle ID %d: Devices 
connected to this active cable will not be seen\n",
  ioc->name, 
ActiveCableEventData->ReceptacleID);

I believe MPT3SAS_FMT is unnecessary obfuscation and
it should just be replaced by "%s: " everywhere.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 12/12] scsi: ufs: Improve fatal error logs

[Joe Perches]

On Mon, 2016-12-12 at 16:56 -0800, Subhash Jadavani wrote:
> Errors such as UIC error, illegal OCS values, and others may require
> more information for debugging. Such information could be hibern8 events,
> events sequences, recoverable errors, error history, and more.
[]
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
[]
> @@ -346,6 +346,37 @@ static inline void ufshcd_cond_add_cmd_trace(struct 
> ufs_hba *hba,
[]
> +static void ufshcd_print_uic_err_hist(struct ufs_hba *hba,
> + struct ufs_uic_err_reg_hist *err_hist, char *err_name)
> +{
> + int i;
> +
> + for (i = 0; i < UIC_ERR_REG_HIST_LENGTH; i++) {
> + int p = (i + err_hist->pos - 1) % UIC_ERR_REG_HIST_LENGTH;
> +
> + if (err_hist->reg[p] == 0)
> + continue;
> + dev_err(hba->dev, "%s[%d] = 0x%x at %lld us", err_name, i,
> + err_hist->reg[p], ktime_to_us(err_hist->tstamp[p]));

Please consistently use a terminating \n

> + }
> +}
> +
>  static void ufshcd_print_host_regs(struct ufs_hba *hba)
>  {
>   /*
> @@ -362,6 +393,21 @@ static void ufshcd_print_host_regs(struct ufs_hba *hba)
>   dev_err(hba->dev,
>   "hba->outstanding_reqs = 0x%x, hba->outstanding_tasks = 0x%x",
>   (u32)hba->outstanding_reqs, (u32)hba->outstanding_tasks);
> + dev_err(hba->dev,
> + "last_hibern8_exit_tstamp at %lld us, hibern8_exit_cnt = %d",

etc...

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi/qla2xxx: label endian-ness for many fields

[Joe Perches]

On Fri, 2016-12-09 at 22:45 +0200, Michael S. Tsirkin wrote:
> This adds endian-ness labels for lots of qla structs.
> Doing this cuts down number of sparse warnings from ~1700 to ~1400.
> Will help find and resolve some of real issues down the road.
> 
> Signed-off-by: Michael S. Tsirkin 
> 
> ---
> 
> Compile-tested only.
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 73b12e4..a4d3071 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -1159,28 +1159,28 @@ typedef struct {
>*/
>   uint8_t  firmware_options[2];
>  
> - uint16_t frame_payload_size;
> - uint16_t max_iocb_allocation;
> - uint16_t execution_throttle;
> + __le16 frame_payload_size;
> + __le16 max_iocb_allocation;
> + __le16 execution_throttle;

Shouldn't all these _not_ have the leading __?
Perhaps the uint8_t uses should be converted to u8 as well.

[etc...]

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sd: assign appropriate log level

[Joe Perches]

On Mon, 2016-10-17 at 09:51 -0700, David Singleton wrote:
> From: Shikhar Dogra 
> 
> Reduce chatter on console for usb hotplug.
> KERN_ERR is too high severity for these messages, moving them
> to KERN_WARNING

Perhaps KERN_NOTICE is more appropriate.
That's the level for most of these sd_first_printk already.

> USB devices never have a Caching Mode page, it doesn't make
> sense to make it an error when you have tons of USB devices where
> the print is useless, and not an error.
> 
> For second message, the condition is not an error. The existing
> workaround of assuming a write through cache doesn't limit
> functionality in any way.
> 
> Cc: xe-ker...@external.cisco.com
> Signed-off-by: Shikhar Dogra 
> Signed-off-by: David Singleton 
> ---
>  drivers/scsi/sd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 51e5629..ab7bfe3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2540,7 +2540,7 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned 
> char *buffer)
>   }
>   }
>  
> - sd_first_printk(KERN_ERR, sdkp, "No Caching mode page found\n");
> + sd_first_printk(KERN_WARNING, sdkp, "No Caching mode page 
> found\n");
>   goto defaults;
>  
>   Page_found:
> @@ -2594,7 +2594,7 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned 
> char *buffer)
>   "Assuming drive cache: write back\n");
>   sdkp->WCE = 1;
>   } else {s
> - sd_first_printk(KERN_ERR, sdkp,
> + sd_first_printk(KERN_WARNING, sdkp,
>   "Assuming drive cache: write through\n");
>   sdkp->WCE = 0;
>   }
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/26] constify local structures

[Joe Perches]

On Sun, 2016-09-11 at 15:05 +0200, Julia Lawall wrote:
> Constify local structures.

Thanks Julia.

A few suggestions & questions:

Perhaps the script should go into scripts/coccinelle/
so that future cases could be caught by the robot
and commit message referenced by the patch instances.

Can you please compile the files modified using the
appropriate defconfig/allyesconfig and show the
movement from data to const by using
$ size .new/old
and include that in the changelogs (maybe next time)?

Is it possible for a rule to trace the instances where
an address of a struct or struct member is taken by
locally defined and declared function call where the
callee does not modify any dereferenced object?

ie:

struct foo {
int bar;
char *baz;
};

struct foo qux[] = {
{ 1, "description 1" },
{ 2, "dewcription 2" },
[ n, "etc" ]...,
};

void message(struct foo *msg)
{
printk("%d %s\n", msg->bar, msg->baz);
}

where some code uses

message(qux[index]);

So could a coccinelle script change:

struct foo qux[] = { to const struct foo quz[] = {

and

void message(struct foo *msg) to void message(const struct foo *msg)

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] wd719x: Remove last declaration using DEFINE_PCI_DEVICE_TABLE

[Joe Perches]

Convert it to the preferred const struct pci_device_id instead.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/wd719x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index e3da1a2..2a9da2e 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -962,7 +962,7 @@ static void wd719x_pci_remove(struct pci_dev *pdev)
scsi_host_put(sh);
 }
 
-static DEFINE_PCI_DEVICE_TABLE(wd719x_pci_table) = {
+static const struct pci_device_id wd719x_pci_table[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_WD, 0x3296) },
{}
 };
-- 
2.10.0.rc2.1.gb2aa91d

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] Remove last use and references to DEFINE_PCI_DEVICE_TABLE

[Joe Perches]

Joe Perches (2):
  wd719x: Remove last declaration using DEFINE_PCI_DEVICE_TABLE
  treewide: Remove references to the now unnecessary DEFINE_PCI_DEVICE_TABLE

 Documentation/PCI/pci.txt | 1 -
 drivers/scsi/wd719x.c | 2 +-
 include/linux/pci.h   | 9 -
 scripts/checkpatch.pl | 9 -
 scripts/tags.sh   | 1 -
 5 files changed, 1 insertion(+), 21 deletions(-)

-- 
2.10.0.rc2.1.gb2aa91d

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] be2iscsi: Use a more current logging style

[Joe Perches]

On Wed, 2016-08-17 at 09:20 +0530, Jitendra Bhivare wrote:
> > 
> > -Original Message-
> > From: Joe Perches [mailto:j...@perches.com]
> > Sent: Tuesday, August 16, 2016 3:57 PM
> > To: Jitendra Bhivare; Christophe JAILLET; Jayamohan Kallickal; Ketan
> Mukadam
> > 
> > Cc: Bart Van Assche; James E.J. Bottomley; Martin K. Petersen; linux-
> > s...@vger.kernel.org; linux-ker...@vger.kernel.org
> > Subject: Re: [PATCH] be2iscsi: Use a more current logging style
> > 
> > On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> > > 
> > > Thanks Joe for taking this up. It has been pending for long time from
> > > our side.
> > Thanks, not a problem, it took ~10 minutes.
> > 
> > There was a bit of an issue about your reply though.
> > 
> > First there was ~50 k of quoted stuff without any content
> > 
> > > 
> > > [ hundreds and hundreds of quoted lines ]
> > and then this happened:
> > 
> > > 
> > > > 
> > > > diff --git a/drivers/scsi/be2iscsi/be_main.h
> > > b/drivers/scsi/be2iscsi/be_main.h
> > > > 
> > > > 
> > > > index aa9c682..7cce6e3 100644
> > > > --- a/drivers/scsi/be2iscsi/be_main.h
> > > > +++ b/drivers/scsi/be2iscsi/be_main.h
> > > > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> > > >  #define BEISCSI_LOG_CONFIG 0x0020  /* CONFIG Code Path */
> > > >  #define BEISCSI_LOG_ISCSI  0x0040  /* SCSI/iSCSI Protocol
> related
> > 
> > > 
> > > Logs */
> > > > 
> > > > 
> > > > 
> > > > -#define __beiscsi_log(phba, level, fmt, ...)
> \
> > 
> > > 
> > > > 
> > > > -   shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > > > -
> > > > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...)   
> > > > \
> > > > +#define beiscsi_printk(level, phba, mask, fmt, ...)
> \
> > 
> > > 
> > > > 
> > > >  do {
> > \
> > > 
> > > > 
> > > > -   uint32_t log_value = phba->attr_log_enable; 
> > > > \
> > > > -   if (((mask) & log_value) || (level[1] <= '3'))  
> > > > \
> > > > -   __beiscsi_log(phba, level, prefix "_%d: " fmt,  
> > > > \
> > > > -     __LINE__, ##__VA_ARGS__); 
> > > > \
> > > > +   if ((mask) & (phba)->attr_log_enable)   
> > > > \
> > > > +   shost_printk(level, phba->shost,
> > > > \
> > > [JB] PCI dev_printk would be more useful with SCSI host_no included by
> > > default in the message.
> > This is a good note that seems simple enough, but I almost missed this.
> > 
> > Given the reply at the top and the _very_ long uncommented quoted block,
> I just
> > 
> > about assumed it was a useless block quote that you didn't bother to
> trim.
> > 
> > 
> > Please make it easier to find your replies and notes by deleting
> irrelevant quoted
> > 
> > stuff.
> > 
> > Also, I think I misread the code.
> > 
> > The original code is <= '3' i.e.: show all KERN_ERR.
> > That is not correct in the new code.
> > 
> > I don't know the code well and don't have a test bed with the hardware.
> > 
> > Is it possible for a beiscsi_ message to be called before
> phba->pcidev is
> > 
> > set to a valid value in beiscsi_hba_alloc?   It appears the code is
> careful to only
> > 
> > use dev_ logging calls before probe.
> [JB] KERN_ERR messages need to be logged irrespective of the masks.
> I understand, that in some places, mask is unnecessarily passed.
> I had made sure to call __beiscsi_log in some places.

I did as well.

> Can we please keep it that way? So beiscsi_err calls dev_err directly or
> is replaced with dev_err.

No worries, I'll respin the series after Christophe's
patches are applied.

> It's safe to assume pcidev will be valid for all beiscsi_log calls.
> Will test your change on my setup before ack'ing.

Don't bother until you get another patchset.

I suggest you fix your email client when sending
replies to me and to lkml.

What I received is very difficult to read due to
the odd line wrapping.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] be2iscsi: Logging neatening

[Joe Perches]

On Wed, 2016-08-17 at 01:19 +, Bart Van Assche wrote:
> On 08/14/16 10:29, Joe Perches wrote:
> > On Sun, 2016-08-14 at 17:09 +, Bart Van Assche wrote:
> > > My primary concern is how to enable and disable log messages from user
> > > space.
[]
> > I think you are looking for a system wide equivalent
> > for the ethtool/netif_ mechanism.
> > 
> > Nothing like that exists currently.
> > 
> > Some code uses a bitmask/and, other code uses a
> > level/comparison.
[]
> As far as I can see all that the ethtool msglevel API implements is a 
> mechanism to query and set the log level from user space. What various 
> SCSI drivers implement is not a log level but a log mask mechanism. How 
> about the following approach to associate a name with each bit in a log 
> mask, to export these names to user space and to make it possible to 
> enable/disable messages per log category:
> * Introduce a variant of pr_debug() that allows to specify a textual
>    representation of the log category (a short string without spaces).
> * Make the log category names available in
>    /sys/kernel/debug/dynamic_debug/...
> * Today dynamic debug allows to enable/disable log messages by
>    specifying the source file name, function name, line number, module
>    name and/or format string. My proposal is to make it also possible to
>    enable/disable log messages based on the log category name.

Many of these logging mechanisms are not just debug
facilities.

Perhaps a dynamic_debug control would be inappropriate.

There have also been various custom scsi log level
facilities like the blogic_msg for the very old
BusLogic blogic_msg.

These functions also sometimes write into some
device-specific buffer.

Perhaps the largest problem, if this is to be scsi only
rather than system wide, is finding out what and how
the various bits in a mask should be used.


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] be2iscsi: Use a more current logging style

[Joe Perches]

On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> Thanks Joe for taking this up. It has been pending for long time from our
> side.

Thanks, not a problem, it took ~10 minutes.

There was a bit of an issue about your reply though.

First there was ~50 k of quoted stuff without any content

> [ hundreds and hundreds of quoted lines ]

and then this happened:

> > diff --git a/drivers/scsi/be2iscsi/be_main.h
> b/drivers/scsi/be2iscsi/be_main.h
> > 
> > index aa9c682..7cce6e3 100644
> > --- a/drivers/scsi/be2iscsi/be_main.h
> > +++ b/drivers/scsi/be2iscsi/be_main.h
> > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> >  #define BEISCSI_LOG_CONFIG 0x0020  /* CONFIG Code Path */
> >  #define BEISCSI_LOG_ISCSI  0x0040  /* SCSI/iSCSI Protocol related
> Logs */
> > 
> > 
> > -#define __beiscsi_log(phba, level, fmt, ...)   
> > \
> > -   shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > -
> > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...)   \
> > +#define beiscsi_printk(level, phba, mask, fmt, ...)
> > \
> >  do {   
> > \
> > -   uint32_t log_value = phba->attr_log_enable; \
> > -   if (((mask) & log_value) || (level[1] <= '3'))  \
> > -   __beiscsi_log(phba, level, prefix "_%d: " fmt,  \
> > -     __LINE__, ##__VA_ARGS__); \
> > +   if ((mask) & (phba)->attr_log_enable)   \
> > +   shost_printk(level, phba->shost,\
> [JB] PCI dev_printk would be more useful with SCSI host_no included by
> default in the message.

This is a good note that seems simple enough, but I almost
missed this.

Given the reply at the top and the _very_ long uncommented
quoted block, I just about assumed it was a useless block
quote that you didn't bother to trim.

Please make it easier to find your replies and notes by
deleting irrelevant quoted stuff.

Also, I think I misread the code.

The original code is <= '3' i.e.: show all KERN_ERR.
That is not correct in the new code.

I don't know the code well and don't have a test bed with
the hardware.

Is it possible for a beiscsi_ message to be called
before phba->pcidev is set to a valid value in
beiscsi_hba_alloc?   It appears the code is careful to
only use dev_ logging calls before probe.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] be2iscsi: Use a more current logging style

[Joe Perches]

shost_printk macros are converted to beiscsi_ for  a
more current logging style.

Consolidate the masked tests into a beiscsi_printk macro and
use that to call shost_printk.

Convert the __beiscsi_log macros to use shost_print directly.

Miscellanea:

o Remove the file prefix identifier and use kbasename(__FILE__)
  and stringify(__LINE__) to reduce code size in beiscsi_printk
o Realign arguments

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/be2iscsi/be_cmds.c  | 142 
 drivers/scsi/be2iscsi/be_iscsi.c | 223 ++--
 drivers/scsi/be2iscsi/be_main.c  | 733 +++
 drivers/scsi/be2iscsi/be_main.h  |  20 +-
 drivers/scsi/be2iscsi/be_mgmt.c  | 270 +++---
 5 files changed, 668 insertions(+), 720 deletions(-)

diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
index a51bc0e..574d2f4 100644
--- a/drivers/scsi/be2iscsi/be_cmds.c
+++ b/drivers/scsi/be2iscsi/be_cmds.c
@@ -95,8 +95,8 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
}
 
if ((status & 0x8000) || (!num_loop)) {
-   beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
-   "BC", "Failed in be_chk_reset_complete status = 
0x%x\n",
+   beiscsi_err(phba, BEISCSI_LOG_INIT,
+   "Failed in be_chk_reset_complete status = 0x%x\n",
status);
return -EIO;
}
@@ -135,9 +135,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
 
spin_lock_bh(>ctrl.mcc_lock);
if (mccq->used == mccq->len) {
-   beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
+   beiscsi_err(phba, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
-   "BC", "MCC queue full: WRB used %u tag avail %u\n",
+   "MCC queue full: WRB used %u tag avail %u\n",
mccq->used, phba->ctrl.mcc_tag_available);
goto alloc_failed;
}
@@ -147,9 +147,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
 
tag = phba->ctrl.mcc_tag[phba->ctrl.mcc_alloc_index];
if (!tag) {
-   beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
+   beiscsi_err(phba, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
-   "BC", "MCC tag 0 allocated: tag avail %u alloc 
index %u\n",
+   "MCC tag 0 allocated: tag avail %u alloc index 
%u\n",
phba->ctrl.mcc_tag_available,
phba->ctrl.mcc_alloc_index);
goto alloc_failed;
@@ -263,10 +263,10 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
set_bit(MCC_TAG_STATE_TIMEOUT,
>ctrl.ptag_state[tag].tag_state);
 
-   beiscsi_log(phba, KERN_ERR,
+   beiscsi_err(phba,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
-   "BC", "MBX Cmd Completion timed out\n");
+   "MBX Cmd Completion timed out\n");
return -EBUSY;
}
 
@@ -289,22 +289,22 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
}
 
if (status || addl_status) {
-   beiscsi_log(phba, KERN_WARNING,
-   BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
-   BEISCSI_LOG_CONFIG,
-   "BC", "MBX Cmd Failed for Subsys : %d Opcode : %d 
with Status : %d and Extd_Status : %d\n",
-   mbx_hdr->subsystem,
-   mbx_hdr->opcode,
-   status, addl_status);
+   beiscsi_warn(phba,
+BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
+BEISCSI_LOG_CONFIG,
+"MBX Cmd Failed for Subsys : %d Opcode : %d with 
Status : %d and Extd_Status : %d\n",
+mbx_hdr->subsystem,
+mbx_hdr->opcode,
+status, addl_status);
rc = -EIO;
if (status == MCC_STATUS_INSUFFICIENT_BUFFER) {
mbx_resp_hdr = (struct be_cmd_resp_hdr *) mbx_hdr;
-   beiscsi_log(phba, KERN_WARNING,
-   BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
-   BEISCSI_LOG_CONFIG,
-   "BC", "Insufficient Buffer Error Resp_Len : 
%d Actual_Resp_Len : %d\n",
- 

Re: [PATCH 0/2] be2iscsi: Logging neatening

[Joe Perches]

On Sun, 2016-08-14 at 17:09 +, Bart Van Assche wrote:

> My primary concern is how to enable and disable log messages from user 
> space. Many drivers define their own logging macros and export a bitmask 
> that allows to enable and disable logging messages per category. These 
> bitmask control mechanisms are annoying because figuring out what bit 
> controls which message category requires a search through the driver 
> source code. I'd like to see all these custom logging macros disappear 
> and being replaced by a single mechanism. The "dynamic debug" mechanism 
> e.g. is in my opinion much easier to use than the different custom 
> logging mechanisms.

Dynamic debug doesn't have a bitmask function and
still requires looking through the code for lines
and format strings.

I think you are looking for a system wide equivalent
for the ethtool/netif_ mechanism.

Nothing like that exists currently.

Some code uses a bitmask/and, other code uses a
level/comparison.

Care to propose something?

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] be2iscsi: Logging neatening

[Joe Perches]

On Sun, 2016-08-14 at 14:34 +, Bart Van Assche wrote:
> On 08/13/16 13:42, Joe Perches wrote:
> > Joe Perches (2):
> >   be2iscsi: Coalesce split strings and formats
> >   be2iscsi: Use a standard logging style
> Hello Joe,

Hello Bart.

> As one can see in be_main.h the "level" argument of macro beiscsi_log() 
> is ignored for log levels KERN_EMERG, KERN_ALERT, KERN_CRIT and
> KERN_ERR. So for these log levels beiscsi_log() is a synonym of 
> shost_printk(). Have you considered to replace beiscsi_log() with 
> shost_printk() for these log levels and additionally to change 
> beiscsi_log() for the other log levels into pr_debug()? pr_debug() 
> statements namely already can be enabled and disabled at runtime. If the 
> BEISCSI_LOG_* log category would be embedded in the log text that would 
> allow to eliminate the phba->attr_log_enable structure member. 
> Additionally, pr_debug() has a facility for displaying the source file 
> name and the line number. That would allow to leave out __LINE__ from 
> be2iscsi log statements. I don't think it is useful to have that line 
> number in non-debug be2iscsi log statements.

My main consideration for submitting a patch at all
was removing the apparent format/argument mismatches.

As far as I can grep, only KERN_ERR, KERN_WARNING and
KERN_INFO are actually used by be2iscsi today.

I agree with the removal of __LINE__ from the macros
as its utility is generally pretty low.

Besides, using stringify(__LINE__) is almost always
smaller object code than a format with "%d", __LINE__.

Prefixes like "BC" and "BS" are __FILE__ equivalents,
and could be removed as well with something like
"%s, kbasename(__FILE__)" used if _really_ desired.

I have no issue with defining and using beiscsi_
equivalents to shost_printks.

I think the test inside beiscsi_log is better removed
with multiple specific beiscsi_ calls used.

I don't know why any KERN_ERR should ever be masked,
but perhaps something like:

#define beiscsi_printk(level, phba, mask, fmt, ...) \
do {\
if ((mask) & (phba)->attr_log_enable)   \
shost_printk(level, phba->shost, fmt, ##__VA_ARGS__); \
} while (0)

#define beiscsi_err(phba, mask, fmt, ...)   \
beiscsi_printk(KERN_ERR, phba, mask, fmt, ##__VA_ARGS__)
#define beiscsi_warn(phba, mask, fmt, ...)  \
beiscsi_printk(KERN_WARNING, phba, mask, fmt, ##__VA_ARGS__)
#define beiscsi_info(phba, mask, fmt, ...)  \
beiscsi_printk(KERN_INFO, phba, mask, fmt, ##__VA_ARGS__)

with a sed of the .c files:

$ sed -i 's/beiscsi_log(phba, KERN_ERR/beiscsi_err(phba/g' 
drivers/scsi/be2iscsi/*.c
$ sed -i 's/beiscsi_log(phba, KERN_WARNING/beiscsi_warn(phba/g' 
drivers/scsi/be2iscsi/*.c
$ sed -i 's/beiscsi_log(phba, KERN_INFO/beiscsi_info(phba/g' 
drivers/scsi/be2iscsi/*.c

with argument realignment of those lines.

All of these are of course up to the actual maintainers of be2iscsi.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] be2iscsi: Use a standard logging style

[Joe Perches]

Neaten all the beiscsi_log uses.

Remove the leading 'B_%d" prefixes and make the
format and arguments match without an implied __LINE__.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/be2iscsi/be_cmds.c  |  54 +++
 drivers/scsi/be2iscsi/be_iscsi.c | 100 ++--
 drivers/scsi/be2iscsi/be_main.c  | 332 +++
 drivers/scsi/be2iscsi/be_main.h  |  19 +--
 drivers/scsi/be2iscsi/be_mgmt.c  |  98 ++--
 5 files changed, 300 insertions(+), 303 deletions(-)

diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
index cefa342..a51bc0e 100644
--- a/drivers/scsi/be2iscsi/be_cmds.c
+++ b/drivers/scsi/be2iscsi/be_cmds.c
@@ -96,7 +96,7 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
 
if ((status & 0x8000) || (!num_loop)) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
-   "BC_%d : Failed in be_chk_reset_complete status = 
0x%x\n",
+   "BC", "Failed in be_chk_reset_complete status = 
0x%x\n",
status);
return -EIO;
}
@@ -137,7 +137,7 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
if (mccq->used == mccq->len) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
-   "BC_%d : MCC queue full: WRB used %u tag avail 
%u\n",
+   "BC", "MCC queue full: WRB used %u tag avail %u\n",
mccq->used, phba->ctrl.mcc_tag_available);
goto alloc_failed;
}
@@ -149,7 +149,7 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
if (!tag) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
-   "BC_%d : MCC tag 0 allocated: tag avail %u alloc 
index %u\n",
+   "BC", "MCC tag 0 allocated: tag avail %u alloc 
index %u\n",
phba->ctrl.mcc_tag_available,
phba->ctrl.mcc_alloc_index);
goto alloc_failed;
@@ -266,7 +266,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_ERR,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
-   "BC_%d : MBX Cmd Completion timed out\n");
+   "BC", "MBX Cmd Completion timed out\n");
return -EBUSY;
}
 
@@ -292,7 +292,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
-   "BC_%d : MBX Cmd Failed for Subsys : %d Opcode : %d 
with Status : %d and Extd_Status : %d\n",
+   "BC", "MBX Cmd Failed for Subsys : %d Opcode : %d 
with Status : %d and Extd_Status : %d\n",
mbx_hdr->subsystem,
mbx_hdr->opcode,
status, addl_status);
@@ -302,7 +302,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
-   "BC_%d : Insufficient Buffer Error Resp_Len 
: %d Actual_Resp_Len : %d\n",
+   "BC", "Insufficient Buffer Error Resp_Len : 
%d Actual_Resp_Len : %d\n",
mbx_resp_hdr->response_length,
mbx_resp_hdr->actual_resp_len);
rc = -EAGAIN;
@@ -341,7 +341,7 @@ static int beiscsi_process_mbox_compl(struct be_ctrl_info 
*ctrl,
if (!compl->flags) {
beiscsi_log(phba, KERN_ERR,
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
-   "BC_%d : BMBX busy, no completion\n");
+   "BC", "BMBX busy, no completion\n");
return -EBUSY;
}
compl->flags = le32_to_cpu(compl->flags);
@@ -363,7 +363,7 @@ static int beiscsi_process_mbox_compl(struct be_ctrl_info 
*ctrl,
return 0;
 
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
-   "BC_%d : error in cmd completion: Subsystem : %d Opcode : 
%d status(compl/extd)=%d/%d\n",
+   "BC", "e

[PATCH 1/2] be2iscsi: Coalesce split strings and formats

[Joe Perches]

Split strings are not preferred for ease of grep.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/scsi/be2iscsi/be_cmds.c  |  15 ++--
 drivers/scsi/be2iscsi/be_iscsi.c |  33 +++
 drivers/scsi/be2iscsi/be_main.c  | 180 ++-
 drivers/scsi/be2iscsi/be_mgmt.c  |   3 +-
 4 files changed, 85 insertions(+), 146 deletions(-)

diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
index a55eaee..cefa342 100644
--- a/drivers/scsi/be2iscsi/be_cmds.c
+++ b/drivers/scsi/be2iscsi/be_cmds.c
@@ -96,8 +96,8 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
 
if ((status & 0x8000) || (!num_loop)) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
-   "BC_%d : Failed in be_chk_reset_complete"
-   "status = 0x%x\n", status);
+   "BC_%d : Failed in be_chk_reset_complete status = 
0x%x\n",
+   status);
return -EIO;
}
 
@@ -292,9 +292,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
-   "BC_%d : MBX Cmd Failed for "
-   "Subsys : %d Opcode : %d with "
-   "Status : %d and Extd_Status : %d\n",
+   "BC_%d : MBX Cmd Failed for Subsys : %d Opcode : %d 
with Status : %d and Extd_Status : %d\n",
mbx_hdr->subsystem,
mbx_hdr->opcode,
status, addl_status);
@@ -304,8 +302,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
-   "BC_%d : Insufficient Buffer Error "
-   "Resp_Len : %d Actual_Resp_Len : %d\n",
+   "BC_%d : Insufficient Buffer Error Resp_Len 
: %d Actual_Resp_Len : %d\n",
mbx_resp_hdr->response_length,
mbx_resp_hdr->actual_resp_len);
rc = -EAGAIN;
@@ -1035,8 +1032,8 @@ int beiscsi_cmd_q_destroy(struct be_ctrl_info *ctrl, 
struct be_queue_info *q,
int status;
 
beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_INIT,
-   "BC_%d : In beiscsi_cmd_q_destroy "
-   "queue_type : %d\n", queue_type);
+   "BC_%d : In beiscsi_cmd_q_destroy queue_type : %d\n",
+   queue_type);
 
mutex_lock(>mbox_lock);
memset(wrb, 0, sizeof(*wrb));
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index 09f89a3..d28ac23 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -70,9 +70,8 @@ struct iscsi_cls_session *beiscsi_session_create(struct 
iscsi_endpoint *ep,
 
if (cmds_max > beiscsi_ep->phba->params.wrbs_per_cxn) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
-   "BS_%d : Cannot handle %d cmds."
-   "Max cmds per session supported is %d. Using %d."
-   "\n", cmds_max,
+   "BS_%d : Cannot handle %d cmds.Max cmds per session 
supported is %d. Using %d.\n",
+   cmds_max,
beiscsi_ep->phba->params.wrbs_per_cxn,
beiscsi_ep->phba->params.wrbs_per_cxn);
 
@@ -139,8 +138,8 @@ beiscsi_conn_create(struct iscsi_cls_session *cls_session, 
u32 cid)
phba = iscsi_host_priv(shost);
 
beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_CONFIG,
-   "BS_%d : In beiscsi_conn_create ,cid"
-   "from iscsi layer=%d\n", cid);
+   "BS_%d : In beiscsi_conn_create ,cidfrom iscsi layer=%d\n",
+   cid);
 
cls_conn = iscsi_conn_setup(cls_session, sizeof(*beiscsi_conn), cid);
if (!cls_conn)
@@ -248,8 +247,7 @@ static int beiscsi_create_ipv4_iface(struct beiscsi_hba 
*phba)
  0, 0);
if (!phba->ipv4_iface) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
-   "BS_%d : Could not "
-   "create default IPv4 address.\n");
+   "BS_%d : Could not create default IPv4 address.\n"

[PATCH 0/2] be2iscsi: Logging neatening

[Joe Perches]

Joe Perches (2):
  be2iscsi: Coalesce split strings and formats
  be2iscsi: Use a standard logging style

 drivers/scsi/be2iscsi/be_cmds.c  |  61 +++---
 drivers/scsi/be2iscsi/be_iscsi.c | 115 ++-
 drivers/scsi/be2iscsi/be_main.c  | 398 +--
 drivers/scsi/be2iscsi/be_main.h  |  19 +-
 drivers/scsi/be2iscsi/be_mgmt.c  |  99 +-
 5 files changed, 314 insertions(+), 378 deletions(-)

-- 
2.8.0.rc4.16.g56331f8

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] be2iscsi: Fix some error messages

[Joe Perches]

On Sat, 2016-08-13 at 09:41 -0700, Joe Perches wrote:
> On Sat, 2016-08-13 at 14:31 +0200, Christophe JAILLET wrote:
> > Le 13/08/2016 à 13:35, Joe Perches a écrit :
> > > > @@ -268,7 +268,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
> > > >     _cmd.dma);
> > > >     if (nonemb_cmd.va == NULL) {
> > > >     beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
> > > > -   "BM_%d : Failed to allocate memory for"
> > > > +   "BM_%d : Failed to allocate memory for "
> > > >     "mgmt_invalidate_icds\n");

This is the first time I've looked at the beiscsi_log macro.

It sure is odd and undesirable.

It's _very_ not nice to have a format string take an implied
__LINE__ argument.

It'd be much more intelligible to take the first bit as a
separate string, concatenate it in the macro with "_%d: "
and __LINE__ (if that's really useful, I think it's not)
and emit that as the format.

Something like:

diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
index 30a4606..3f0fbbf 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -1084,11 +1084,12 @@ struct hwi_context_memory {
 #define __beiscsi_log(phba, level, fmt, arg...) \
    shost_printk(level, phba->shost, fmt, __LINE__, ##arg)
 
-#define beiscsi_log(phba, level, mask, fmt, arg...) \
-do { \
-   uint32_t log_value = phba->attr_log_enable; \
-   if (((mask) & log_value) || (level[1] <= '3')) \
-   __beiscsi_log(phba, level, fmt, ##arg); \
-} while (0);
+#define beiscsi_log(phba, level, mask, prefix, fmt, ...)   \
+do {   \
+   uint32_t log_value = phba->attr_log_enable; \
+   if (((mask) & log_value) || (level[1] <= '3'))  \
+   __beiscsi_log(phba, level, prefix "_%d: " fmt,  \
+     ##__VA_ARGS__);   \
+} while (0)
 
 #endif

So these beiscsi_log uses become something like:

beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
"BM", "Failed to allocate memory for 
mgmt_invalidate_icds\n");

and the format and its arguments match.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] be2iscsi: Fix some error messages

[Joe Perches]

On Sat, 2016-08-13 at 14:31 +0200, Christophe JAILLET wrote:
> Le 13/08/2016 à 13:35, Joe Perches a écrit :
> > > @@ -268,7 +268,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
> > >   _cmd.dma);
> > >   if (nonemb_cmd.va == NULL) {
> > >   beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
> > > - "BM_%d : Failed to allocate memory for"
> > > + "BM_%d : Failed to allocate memory for "
> > >   "mgmt_invalidate_icds\n");
> > doesn't match commit log as no coalescing/concatenation
> > is done.
> > 
> > There are many of these.
> > 
> I have *only* fixed the one reported by checkpatch and left the others 
> unchanged.
> 
> My initial proposal was to fix incorrect strings, without modifying too 
> much the code. So I decided to do the minimum of changes.
> 
> Should I resubmitted with:
> - all strings *in the patch* concatenated?
> - all strings *in the file*" concatenated?

Hello Christophe

You don't _have_ to do anything.

I think the commit message is misleading.

You could submit another patch that does
the equivalent of:

$ ./scripts/checkpatch.pl --types=SPLIT_STRING --fix-inplace 
drivers/scsi/be2iscsi/be_main.c

with the appropriate commit message

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] be2iscsi: Fix some error messages

[Joe Perches]

On Sat, 2016-08-13 at 09:20 +0200, Christophe JAILLET wrote:
> This fixes:
[]
>    - concatenate strings on the same line to fix checkpatch warnings
[]
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
[]
> @@ -268,7 +268,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
>   _cmd.dma);
>   if (nonemb_cmd.va == NULL) {
>   beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
> - "BM_%d : Failed to allocate memory for"
> + "BM_%d : Failed to allocate memory for "
>   "mgmt_invalidate_icds\n");

doesn't match commit log as no coalescing/concatenation
is done.

There are many of these.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] lib: string: add functions to case-convert strings

[Joe Perches]

On Tue, 2016-07-05 at 15:36 -0700, Markus Mayer wrote:
> On 5 July 2016 at 15:14, Joe Perches <j...@perches.com> wrote:
> > On Tue, 2016-07-05 at 13:47 -0700, Markus Mayer wrote:
> > > This series introduces a family of generic string case conversion
> > > functions. This kind of functionality is needed in several places in
> > > the kernel. Right now, everybody seems to be implementing their own
> > > copy of this functionality.
> > > 
> > > Based on the discussion of the previous version of this series[1] and
> > > the use cases found in the kernel, it does look like having several
> > > flavours of case conversion functions is beneficial. The use cases fall
> > > into three categories:
> > > - copying a string and converting the case while specifying a
> > >   maximum length to mimic strncpy()
> > > - copying a string and converting the case without specifying a
> > >   length to mimic strcpy()
> > > - converting the case of a string in-place (i.e. modifying the
> > >   string that was passed in)
> > > 
> > > Consequently, I am proposing these new functions:
> > > char *strncpytoupper(char *dst, const char *src, size_t len);
> > > char *strncpytolower(char *dst, const char *src, size_t len);
> > > char *strcpytoupper(char *dst, const char *src);
> > > char *strcpytolower(char *dst, const char *src);
> > > char *strtoupper(char *s);
> > > char *strtolower(char *s);
> > I think there isn't much value in anything other
> > than strto.
> > 
> > Using str[n]cpy followed by strto is
> > pretty obvious and rarely used anyway.
> First time around, folks were proposing the "copy" variants when I
> submitted just strtolower() by itself[1]. They just asked for source
> and destination parameters to strtolower(), but looking at the use
> cases that wouldn't have worked so well. Hence it evolved into these 6
> functions.
> 
> Here's a breakdown of how the functions are being used (patches 2-7),
> see also [2]:
> 
> Patch 2: strncpytolower()
> Patch 3: strtolower()
> Patch 4: strncpytolower() and strtolower()
> Patch 5: strtolower()
> Patch 6: strcpytoupper()
> Patch 7: strcpytoupper()
> 
> So it does look like the copy + change case variant is more frequently
> used than just strto.

Are these functions useful?   Not to me, not so much.

None of the functions would have the strcpy performance of
the arch / asm
versions of strcpy and the savings in overall
code isn't significant (or
measured?).

Of course none of the uses are runtime performance important.

This patch also adds always compiled functions that aren't used
in many .configs.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] lib: string: add functions to case-convert strings

[Joe Perches]

On Tue, 2016-07-05 at 13:47 -0700, Markus Mayer wrote:
> This series introduces a family of generic string case conversion
> functions. This kind of functionality is needed in several places in
> the kernel. Right now, everybody seems to be implementing their own
> copy of this functionality.
> 
> Based on the discussion of the previous version of this series[1] and
> the use cases found in the kernel, it does look like having several
> flavours of case conversion functions is beneficial. The use cases fall
> into three categories:
> - copying a string and converting the case while specifying a
>   maximum length to mimic strncpy()
> - copying a string and converting the case without specifying a
>   length to mimic strcpy()
> - converting the case of a string in-place (i.e. modifying the
>   string that was passed in)
> 
> Consequently, I am proposing these new functions:
> char *strncpytoupper(char *dst, const char *src, size_t len);
> char *strncpytolower(char *dst, const char *src, size_t len);
> char *strcpytoupper(char *dst, const char *src);
> char *strcpytolower(char *dst, const char *src);
> char *strtoupper(char *s);
> char *strtolower(char *s);

I think there isn't much value in anything other
than strto.

Using str[n]cpy followed by strto is
pretty obvious and rarely used anyway.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] scsi: wd7000: print sector number as 64-bit

[Joe Perches]

On Tue, 2016-06-21 at 11:02 +0200, Arnd Bergmann wrote:
> Enabling format checking in dprintk() shows that wd7000_biosparam
> uses an incorrect format string for sector_t:

trivia:

> diff --git 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

It'd be nicer if both defines were the same form

#ifdef WD7000_DEBUG
#define dprintk printk
#else
#define dprintk no_printk
#endif


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] VMW_PVSCSI: Change to update maintainer details (name, email)

[Joe Perches]

On Fri, 2016-06-17 at 17:44 +, Jim Gill wrote:
> On 6/16/16, 8:11 PM, "Julian Calaby" <julian.cal...@gmail.com> wrote:
> 
> 
> 
> > 
> > []
> > 
> > > 
> > > > 
> > > > On Fri, Jun 17, 2016 at 12:33 PM, Joe Perches <j...@perches.com> wrote:
> > > []
> > > The question to me is whether or not Jim Gill is
> > > taking over the maintainership of the entire
> > > VMware PVSCSI driver or just a few files of it.
> > As I see it, he's taking over maintainership of all of it: it's only
> > files are drivers/scsi/vmw_pvscsi.[ch] AFAIK.
> This is correct, I am taking over maintainership of the entire vmw_pvscsi 
> driver.

Perhaps a patch like this:

>From e727c6549e3be466ec3c79e919502cb0b9909b03 Mon Sep 17 00:00:00 2001
Message-Id: 
<e727c6549e3be466ec3c79e919502cb0b9909b03.1466186573.git@perches.com>
From: Joe Perches <j...@perches.com>
Date: Fri, 17 Jun 2016 10:56:49 -0700
Subject: [PATCH] vmw_pvscsi: Move into separate directory, Jim Gill is 
MAINTAINER

Separate directories for drivers are generally better.

Miscellanea:

o Update the MAINTAINER entry
o Remove maintainer and FSF addresses from driver files
---
 MAINTAINERS| 5 ++---
 drivers/scsi/Kconfig   | 8 +---
 drivers/scsi/Makefile  | 2 +-
 drivers/scsi/vmware/Kconfig| 7 +++
 drivers/scsi/vmware/Makefile   | 1 +
 drivers/scsi/{ => vmware}/vmw_pvscsi.c | 7 ---
 drivers/scsi/{ => vmware}/vmw_pvscsi.h | 7 ---
 7 files changed, 12 insertions(+), 25 deletions(-)
 create mode 100644 drivers/scsi/vmware/Kconfig
 create mode 100644 drivers/scsi/vmware/Makefile
 rename drivers/scsi/{ => vmware}/vmw_pvscsi.c (99%)
 rename drivers/scsi/{ => vmware}/vmw_pvscsi.h (98%)

diff --git a/MAINTAINERS b/MAINTAINERS
index d174e34..2763582 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12421,12 +12421,11 @@ S:Maintained
 F: drivers/net/vmxnet3/
 
 VMware PVSCSI driver
-M: Arvind Kumar <arvindku...@vmware.com>
+M: Jim Gill <jg...@vmware.com>
 M: VMware PV-Drivers <pv-driv...@vmware.com>
 L: linux-scsi@vger.kernel.org
 S: Maintained
-F: drivers/scsi/vmw_pvscsi.c
-F: drivers/scsi/vmw_pvscsi.h
+F: drivers/scsi/vmware/
 
 VOLTAGE AND CURRENT REGULATOR FRAMEWORK
 M: Liam Girdwood <lgirdw...@gmail.com>
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 1918f54..339c230 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -576,13 +576,7 @@ config SCSI_FLASHPOINT
      substantial, so users of MultiMaster Host Adapters may not
      wish to include it.
 
-config VMWARE_PVSCSI
-   tristate "VMware PVSCSI driver support"
-   depends on PCI && SCSI && X86
-   help
-     This driver supports VMware's para virtualized SCSI HBA.
-     To compile this driver as a module, choose M here: the
-     module will be called vmw_pvscsi.
+source "drivers/scsi/vmware/Kconfig"
 
 config XEN_SCSI_FRONTEND
    tristate "XEN SCSI frontend driver"
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 862ab4e..6cfefaa 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -141,7 +141,7 @@ obj-$(CONFIG_BE2ISCSI)  += libiscsi.o be2iscsi/
 obj-$(CONFIG_SCSI_ESAS2R)  += esas2r/
 obj-$(CONFIG_SCSI_PMCRAID) += pmcraid.o
 obj-$(CONFIG_SCSI_VIRTIO)  += virtio_scsi.o
-obj-$(CONFIG_VMWARE_PVSCSI)+= vmw_pvscsi.o
+obj-$(CONFIG_VMWARE_PVSCSI)+= vmware/
 obj-$(CONFIG_XEN_SCSI_FRONTEND)+= xen-scsifront.o
 obj-$(CONFIG_HYPERV_STORAGE)   += hv_storvsc.o
 obj-$(CONFIG_SCSI_WD719X)  += wd719x.o
diff --git a/drivers/scsi/vmware/Kconfig b/drivers/scsi/vmware/Kconfig
new file mode 100644
index 000..3c0c53b
--- /dev/null
+++ b/drivers/scsi/vmware/Kconfig
@@ -0,0 +1,7 @@
+config VMWARE_PVSCSI
+   tristate "VMware PVSCSI driver support"
+   depends on PCI && SCSI && X86
+   help
+     This driver supports VMware's para virtualized SCSI HBA.
+     To compile this driver as a module, choose M here: the
+     module will be called vmw_pvscsi.
diff --git a/drivers/scsi/vmware/Makefile b/drivers/scsi/vmware/Makefile
new file mode 100644
index 000..ae8d278
--- /dev/null
+++ b/drivers/scsi/vmware/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VMWARE_PVSCSI)+= vmw_pvscsi.o
diff --git a/drivers/scsi/vmw_pvscsi.c b/drivers/scsi/vmware/vmw_pvscsi.c
similarity index 99%
rename from drivers/scsi/vmw_pvscsi.c
rename to drivers/scsi/vmware/vmw_pvscsi.c
index 6164634..eb1229e 100644
--- a/drivers/scsi/vmw_pvscsi.c
+++ b/drivers/scsi/vmware/vmw_pvscsi.c
@@ -12,13 +12,6 @@
  * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
  * NON INFRINGEMENT.  See the GNU General Public Li

Re: [PATCH] VMW_PVSCSI: Change to update maintainer details (name, email)

[Joe Perches]

On Fri, 2016-06-17 at 12:44 +1000, Julian Calaby wrote:
> Hi Joe,

rehi Julian.

> On Fri, Jun 17, 2016 at 12:33 PM, Joe Perches <j...@perches.com> wrote:
[]
> > get_maintainer.pl also has a rarely used "--file-emails" option to
> > scan for what appears to be email addresses in specific files.
> > 
> > $ ./scripts/get_maintainer.pl  -f --file-emails drivers/scsi/vmw_pvscsi.c
> > Arvind Kumar <arvindku...@vmware.com> (maintainer:VMware PVSCSI driver,in 
> > file)
> > VMware PV-Drivers <pv-driv...@vmware.com> (maintainer:VMware PVSCSI driver)
> > "James E.J. Bottomley" <j...@linux.vnet.ibm.com> (maintainer:SCSI SUBSYSTEM)
> > "Martin K. Petersen" <martin.peter...@oracle.com> (maintainer:SCSI 
> > SUBSYSTEM)
> > linux-scsi@vger.kernel.org (open list:VMware PVSCSI driver)
> > linux-ker...@vger.kernel.org (open list)
> > 
> > note the "in file" after Arvind's name
> Didn't know this, however my point stands: the maintainer line in the
> file is redundant if we find maintainers through MAINTAINERS or the
> get_maintainer.pl script.

Yes, I'm not suggesting anything else.  Jim's name
should appear in the MAINTAINERS file somewhere.

The question to me is whether or not Jim Gill is
taking over the maintainership of the entire
VMware PVSCSI driver or just a few files of it.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] VMW_PVSCSI: Change to update maintainer details (name, email)

[Joe Perches]

On Fri, 2016-06-17 at 12:18 +1000, Julian Calaby wrote:
> ./scripts/get_maintainers.pl -f drivers/scsi/vmw_pvscsi.c

just fyi:  the script name is not plural

$ ./scripts/get_maintainer.pl  -f drivers/scsi/vmw_pvscsi.c
Arvind Kumar  (maintainer:VMware PVSCSI driver)
VMware PV-Drivers  (maintainer:VMware PVSCSI driver)
"James E.J. Bottomley"  (maintainer:SCSI SUBSYSTEM)
"Martin K. Petersen"  (maintainer:SCSI SUBSYSTEM)
linux-scsi@vger.kernel.org (open list:VMware PVSCSI driver)
linux-ker...@vger.kernel.org (open list)

and another fyi:

get_maintainer.pl also has a rarely used "--file-emails" option to
scan for what appears to be email addresses in specific files.

$ ./scripts/get_maintainer.pl  -f --file-emails drivers/scsi/vmw_pvscsi.c
Arvind Kumar  (maintainer:VMware PVSCSI driver,in file)
VMware PV-Drivers  (maintainer:VMware PVSCSI driver)
"James E.J. Bottomley"  (maintainer:SCSI SUBSYSTEM)
"Martin K. Petersen"  (maintainer:SCSI SUBSYSTEM)
linux-scsi@vger.kernel.org (open list:VMware PVSCSI driver)
linux-ker...@vger.kernel.org (open list)

note the "in file" after Arvind's name
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

[Joe Perches]

On Thu, 2016-06-16 at 16:20 -0500, Bryant G. Ly wrote:
> This driver is a pick up of the old IBM VIO scsi Target Driver
> that was started by Nick and Fujita 2-4 years ago.
> http://comments.gmane.org/gmane.linux.scsi/90119

(style trivia only, nothing important enough to force a respin
 but nice to fix one day)

Please use git format-patch -M so file renames are more obvious.

> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> +IBM Power Virtual SCSI Device Target Driver
> +M:   Bryant G. Ly 
> +M:   Michael Cyr 
> +L:   linux-scsi@vger.kernel.org
> +L:   target-de...@vger.kernel.org
> +S:   Supportedybe 
> +F:   drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c

Maybe

F:  drivers/scsi/ibmscsi_tgt/

> +F:   drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
> +F:   drivers/scsi/ibmvscsi_tgt/libsrp.c
> +F:   drivers/scsi/ibmvscsi_tgt/libsrp.h

and these become unnecessary

> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
[]
> +/**
> + * ibmvscsis_establish_new_q() - Establish new CRQ queue
> + */

If you use kernel-doc comment style,
please describe the function arguments too.

> +static long ibmvscsis_establish_new_q(struct scsi_info *vscsi,  uint 
> new_state)
> +{
> + long rc = ADAPT_SUCCESS;
> + uint format;
> +
> + vscsi->flags &= PRESERVE_FLAG_FIELDS;
> + vscsi->rsp_q_timer.timer_pops = 0;
> + vscsi->debit = 0;
> + vscsi->credit = 0;
> + rc = vio_enable_interrupts(vscsi->dma_dev);
> + if (rc) {
> + pr_warn("reset_queue: failed to enable interrupts, rc %ld\n",
> + rc);
> + } else {
> + rc = ibmvscsis_check_init_msg(vscsi, );
> + if (rc) {
> + dev_err(>dev, "reset_queue: check_init_msg 
> failed, rc %ld\n",
> + rc);
> + } else if (format == UNUSED_FORMAT &&
> +    new_state == WAIT_CONNECTION) {
> + rc = ibmvscsis_send_init_message(vscsi, INIT_MSG);
> + switch (rc) {
> + case H_SUCCESS:
> + case H_DROPPED:
> + case H_CLOSED:
> + rc = ADAPT_SUCCESS;
> + break;
> +
> + case H_PARAMETER:
> + case H_HARDWARE:
> + break;
> +
> + default:
> + vscsi->state = UNDEFINED;
> + rc = H_HARDWARE;
> + break;
> + }
> + }
> + }
> +
> + return rc;
> +}

This sort of code can have indent reduced by doing

rc = vio_enable_interrupts(...)
if (rc) {
pr_warn(...);
return rc;
}

rc = ibmvscsis_check_init_msg(...)
if (rc) {
dev_err(...);
return rc;
}

if (format == UNUSED_FORMAT && new_state == WAIT_CONNECTION) {
rc = ibmvscsis_send_init_message(vscsi, INIT_MSG);  
switch (rc) {
etc...
}
}

Why use pr_warn and dev_err in the same function?
Shouldn't dev_ be used whenever a struct device is available?

Also it's nice to use %s, __func__ instead of directly
embedding a function like name into the format string.

A lot of the pr_debug uses seem to be function tracing
and could be eliminated altogether.

> + /* can transition from this state to UNCONFIGURING */
> + case UNDEFINED:

Comment style for switch / case labels could not be indented
but start at the same indent level as the case statement.


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

[Joe Perches]

On Wed, 2016-06-15 at 18:41 -0500, Bryant G. Ly wrote:
> The driver provides a virtual SCSI device on IBM Power Servers.
[]
>  MAINTAINERS  |   11 +
>  drivers/scsi/Kconfig |   27 +-
>  drivers/scsi/Makefile|2 +-
>  drivers/scsi/ibmvscsi/Makefile   |4 +
>  drivers/scsi/ibmvscsi/ibmvfc.h   |2 +-
>  drivers/scsi/ibmvscsi/ibmvscsi.h |2 +-
>  drivers/scsi/ibmvscsi/viosrp.h   |  225 --
>  drivers/scsi/ibmvscsi_tgt/Makefile   |4 +
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 4029 
> ++
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |  342 +++
>  drivers/scsi/ibmvscsi_tgt/libsrp.c   |  427 
>  drivers/scsi/ibmvscsi_tgt/libsrp.h   |  123 +
>  drivers/scsi/libsrp.c|  447 
>  include/scsi/libsrp.h|   78 -
>  include/scsi/viosrp.h|  220 ++
>  15 files changed, 5180 insertions(+), 763 deletions(-)
>  delete mode 100644 drivers/scsi/ibmvscsi/viosrp.h
>  create mode 100644 drivers/scsi/ibmvscsi_tgt/Makefile
>  create mode 100644 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
>  create mode 100644 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
>  create mode 100644 drivers/scsi/ibmvscsi_tgt/libsrp.c
>  create mode 100644 drivers/scsi/ibmvscsi_tgt/libsrp.h
>  delete mode 100644 drivers/scsi/libsrp.c
>  delete mode 100644 include/scsi/libsrp.h
>  create mode 100644 include/scsi/viosrp.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -5451,6 +5451,17 @@ S: Supported
>  F:   drivers/scsi/ibmvscsi/ibmvscsi*
>  F:   drivers/scsi/ibmvscsi/viosrp.h
>  
> +IBM Power Virtual SCSI Device Target Driver
> +M:   Bryant G. Ly 
> +M:   Michael Cyr 
> +L:   linux-scsi@vger.kernel.org
> +L:   target-de...@vger.kernel.org
> +S:   Supported
> +F:   drivers/scsi/ibmvscsi/ibmvscsis.c
> +F:   drivers/scsi/ibmvscsi/ibmvscsis.h
> +F:   drivers/scsi/ibmvscsi/libsrp.c
> +F:   drivers/scsi/ibmvscsi/libsrp.h

trivia:

Please make the MAINTAINER pattern entries match the file locations.

This could be done in a separate patch if this doesn't need to be
resubmitted for any other reason.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Use the correct size to set block max sectors

[Joe Perches]

dOn Thu, 2016-05-26 at 17:08 -0700, Long Li wrote:
> The block sector size should be in unit of 512 bytes, not in bytes.

Thanks.  The patch subject should use something like:

[PATCH] sd: Use the correct size to set block max sectors

to show what subsystem is being modified.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
[]
> @@ -2862,9 +2862,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   if (sdkp->opt_xfer_blocks &&
>   sdkp->opt_xfer_blocks <= dev_max &&
>   sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> - sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE)
> - rw_max = q->limits.io_opt =
> + sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
> + q->limits.io_opt =
>   sdkp->opt_xfer_blocks * sdp->sector_size;
> + rw_max = (q->limits.io_opt >> 9);
> + }
>   else
>   rw_max = BLK_DEF_MAX_SECTORS;

And style trivia:  it'd be more kernel style consistent as:

if (...
    sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
q->limits.io_opt = sdkp->opt_xfer_blocks * sdp->sector_size;
rw_max = q->limits.io_opt >> 9;
} else {
rw_max = BLK_DEF_MAX_SECTORS;
}

ie: no parentheses necessary around the shifted value and
    braces around both arms.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ibmvscsis: clean up functions

[Joe Perches]

On Wed, 2016-05-25 at 09:17 -0500, Bryant G. Ly wrote:
> From: bryantly 

Please use your whole name here and for your sign-off like:

From: Bryant G. Ly 
Signed-off-by: Bryant G. Ly 

> This patch removes forward declarations and re-organizes the
> functions within the driver. This patch also fixes MAINTAINERS
> for ibmvscsis.

trivial note:

> diff --git a/drivers/scsi/ibmvscsi/ibmvscsis.c 
> b/drivers/scsi/ibmvscsi/ibmvscsis.c
[]
>  static inline long h_copy_rdma(s64 length, u64 sliobn, u64 slioba,
>      u64 dliobn, u64 dlioba)
>  {
> 

Functions like this would be less indented and less
line wrapped for 80 columns if they were written:

if (!se_cmd->residual_count)
return;

[unindented one level...]

etc...

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

[Joe Perches]

On Tue, 2016-05-24 at 08:52 -0500, Bryant G. Ly wrote:
> From: bgly 
> 
> This initial commit contains WIP of the IBM VSCSI Target Fabric
> Module. It currently supports read/writes, and I have tested
> the ability to create a file backstore with the driver and install
> RHEL VIA NIM and then boot up the partition via filio backstore
> through the driver.

Only trivial notes:

Maybe try checkpatch with the --strict option and see
if any of the additional messages are important to you.

> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -5381,6 +5381,16 @@ S: Supported
>  F:   drivers/scsi/ibmvscsi/ibmvscsi*
>  F:   drivers/scsi/ibmvscsi/viosrp.h
>  
> +IBM Power Virtual SCSI Device Target Driver
> +M:   Bryant G. Ly 
> +L:   linux-scsi@vger.kernel.org
> +L:   target-de...@vger.kernel.org
> +S:   Supported
> +F:   drivers/scsi/ibmvscsi/ibmvscsis.c
> +F:  drivers/scsi/ibmvscsi/ibmvscsis.h
> +F:   drivers/scsi/libsrp.h
> +F:  drivers/scsi/libsrp.c

Please use a tab character consistently after the :

> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
[]
> @@ -847,6 +847,20 @@ config SCSI_IBMVSCSI
>     To compile this driver as a module, choose M here: the
>     module will be called ibmvscsi.
>  
> +config SCSI_IBMVSCSIS
> +     tristate "IBM Virtual SCSI Server support"
> +     depends on PPC_PSERIES && SCSI_SRP && TARGET_CORE
> +     help
> +       This is the IBM POWER Virtual SCSI Target Server
> +
> +  The userspace component needed to initialize the driver and
> +       documentation can be found:

here too.

> +
> +  https://github.com/powervm/ibmvscsis
> +
> +  To compile this driver as a module, choose M here: the
> +       module will be called ibmvstgt.
> +

[]

> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
[]
> @@ -127,7 +127,9 @@ obj-$(CONFIG_SCSI_LASI700)+= 53c700.o lasi700.o
>  obj-$(CONFIG_SCSI_SNI_53C710)+= 53c700.o sni_53c710.o
>  obj-$(CONFIG_SCSI_NSP32) += nsp32.o
>  obj-$(CONFIG_SCSI_IPR)   += ipr.o
> +obj-$(CONFIG_SCSI_SRP)  += libsrp.o
>  obj-$(CONFIG_SCSI_IBMVSCSI)  += ibmvscsi/
> +obj-$(CONFIG_SCSI_IBMVSCSIS)+= ibmvscsi/

and here

> diff --git a/drivers/scsi/ibmvscsi/ibmvscsis.c 
> b/drivers/scsi/ibmvscsi/ibmvscsis.c
[]

> +static int ibmvscsis_probe(struct vio_dev *vdev,
> +    const struct vio_device_id *id);
[...]

It might be nice to rearrange the code to avoid these forward
function declarations.

> +static ssize_t ibmvscsis_tpg_enable_store(struct config_item *item,
> + const char *page, size_t count)
> +{
[]
> > +   ret = kstrtoul(page, 0, );
> + if (ret < 0) {
> + pr_err("Unable to extract ibmvscsis_tpg_store_enable\n");
> + return -EINVAL;
> + }

It might be nicer to add:

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

before any #include to output all the logging messages with
a standardized prefix.  Then all the other logging with an
embedded prefix can have the embedded prefix removed too.

> +
> + if ((tmp != 0) && (tmp != 1)) {
> + pr_err("Illegal value for ibmvscsis_tpg_store_enable: %lu\n",
> + tmp);
> + return -EINVAL;
> + }
> +
> + if (tmp == 1)
> + tport->enabled = true;
> + else
> + tport->enabled = false;

tport->enabled = tmp;

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] megaraid_sas: trivial fix, add missing space in dev_err message

[Joe Perches]

On Mon, 2016-04-25 at 22:58 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Add a missing space in dev_err message, missed because the string
> spans a few lines.

This is a dev_notice() not dev_err().

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c 
> b/drivers/scsi/megaraid/megaraid_sas_base.c
[]
> @@ -3345,7 +3345,7 @@ megasas_internal_reset_defer_cmds(struct 
> megasas_instance *instance)
>   if (!list_empty(>list)) {
>   dev_notice(>pdev->dev, "ERROR while"
>   " moving this cmd:%p, %d %p, it was"
> - "discovered on some list?\n",
> + " discovered on some list?\n",
>   cmd, cmd->sync_cmd, cmd->scmd);
>  
>   list_del_init(>list);

Better would be to coalesce the format, but perhaps
this dev_notice should be dev_err?

dev_notice(>pdev->dev,
   "ERROR while moving this cmd:%p, %d 
%p, it was discovered on some list?\n",
   cmd, cmd->sync_cmd, cmd->scmd);

And the message seems oddly phrased.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cxgbit: fix dma_addr_t printk format

[Joe Perches]

On Sat, 2016-03-05 at 01:34 +0100, Arnd Bergmann wrote:
> On Friday 04 March 2016 16:25:07 Joe Perches wrote:
> > > diff --git a/drivers/target/iscsi/cxgbit/cxgbit_ddp.c 
> > > b/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
[]
> > > @@ -179,7 +179,7 @@ cxgbit_dump_sgl(const char *cap, struct scatterlist 
> > > *sgl, int nents)
> > >   for_each_sg(sgl, sg, nents, i)
> > >   pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma 
> > > 0x%llx, %u\n",
> > >   i, nents, sg, sg->length, sg->offset, sg_page(sg),
> > > - sg_dma_address(sg), sg_dma_len(sg));
> > > + (u64)sg_dma_address(sg), sg_dma_len(sg));
[]
> > You could create a temporary:


for_each_sg(sgl, sg, nents, i) {
dma_addr_t addr = sg_dma_address(sg);


pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma %pad, 
%u\n",
i, nents, sg, sg->length, sg->offset, sg_page(sg),
, sg_dma_len(sg));
}

Sure, but the cast seemed nicer in this case, the result is the same.

Not quite as 0x%llx isn't always the same width and doesn't
have leading 0's like %pad


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cxgbit: fix dma_addr_t printk format

[Joe Perches]

On Sat, 2016-03-05 at 01:04 +0100, Arnd Bergmann wrote:
> The newly added driver prints a dma_addr_t using the %llx format string,
> but that is wrong on most 32-bit architectures:
> 
> drivers/target/iscsi/cxgbit/cxgbit_ddp.c: In function 'cxgbit_dump_sgl':
> drivers/target/iscsi/cxgbit/cxgbit_ddp.c:180:10: error: format '%llx' expects 
> argument of type 'long long unsigned int', but argument 8 has type 
> 'dma_addr_t {aka unsigned int}' [-Werror=format=]
>    pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma 0x%llx, %u\n",
> 
> Unfortunately, we can't use the %pad format string here because
> we are not printing an lvalue, so we have to add a cast to u64, which
> matches the format string on all architectures.
> Signed-off-by: Arnd Bergmann 
> Fixes: c49aa56e556d ("cxgbit: add cxgbit_ddp.c")
> ---
>  drivers/target/iscsi/cxgbit/cxgbit_ddp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/iscsi/cxgbit/cxgbit_ddp.c 
> b/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> index 07e2bc86d0df..d667bc88e21d 100644
> --- a/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> +++ b/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> @@ -179,7 +179,7 @@ cxgbit_dump_sgl(const char *cap, struct scatterlist *sgl, 
> int nents)
>   for_each_sg(sgl, sg, nents, i)
>   pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma 0x%llx, 
> %u\n",
>   i, nents, sg, sg->length, sg->offset, sg_page(sg),
> - sg_dma_address(sg), sg_dma_len(sg));
> + (u64)sg_dma_address(sg), sg_dma_len(sg));
>  }
>  
>  static int cxgbit_ddp_sgl_check(struct scatterlist *sgl, int nents)

You could create a temporary:

    for_each_sg(sgl, sg, nents, i) {
dma_addr_t addr = sg_dma_address(sg);

pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma %pad, 
%u\n",
i, nents, sg, sg->length, sg->offset, sg_page(sg),
, sg_dma_len(sg));
}

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Modified Maintainers list for MPT FUSION DRIVERS.

[Joe Perches]

On Wed, 2016-02-17 at 16:55 +0530, Chaitra P B wrote:
> Signed-off-by: Chaitra P B 
> Suganath prabu Subramani 

Is this supposed to be a signed-off-by line?

A couple questions below.

> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -6659,10 +6659,9 @@ S: Maintained
>  F:   arch/arm/mach-lpc32xx/
>  
>  LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
> -M:   Nagalakshmi Nandigama 
> -M:   Praveen Krishnamoorthy 
> -M:   Sreekanth Reddy 
> -M:   Abhijit Mahajan 
> +M:   Sathya Prakash 
> +M:   Chaitra P B 
> +M:   Suganath Prabu Subramani 
>  L:   mpt-fusionlinux@avagotech.com

Should this L: entry be changed?

>  L:   linux-scsi@vger.kernel.org
>  W:   http://www.lsilogic.com/support

This W: link seems broken/dead.
Should this be updated too?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp

[Joe Perches]

On Sun, 2016-01-03 at 22:05 +, One Thousand Gnomes wrote:
> On Sun, 03 Jan 2016 13:46:22 -0800
> Joe Perches <j...@perches.com> wrote:
> 
> > (using an email address for James that shouldn't bounce)
> > 
> > On Sun, 2016-01-03 at 21:29 +, One Thousand Gnomes wrote:
> > > > > I would beg to differ. As a tool for understanding the
> > > > > differences as you
> > > > > step through the versions it's invaluable.
> > > > 
> > > > This removes intentional formatting that
> > > > makes reading comments easier.
> > > 
> > > And this matters at the end of the patch series because ?
> > 
> > It removes intentional formatting that makes
> > reading comments easier.
> 
> I'm missing something here - the final result of the merge is completely
> and easily readable. Likewise I can work through it and review the code
> and compare them for the platform I care about (Mac68K)
> 
> > And diff -w still works just fine.
> 
> So you think 70+ patches should be redone because of that, when the end
> result is a clean driver anyway ???

Nope, I think patch 69 should simply not be applied.

I don't know and don't care if patches 70 through 78
depend on any changes made by patch 69.

I'd expect no dependencies exist though.

It doesn't matter much in any case.

cheers, Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp

[Joe Perches]

(using an email address for James that shouldn't bounce)

On Sun, 2016-01-03 at 21:29 +, One Thousand Gnomes wrote:
> > > I would beg to differ. As a tool for understanding the
> > > differences as you
> > > step through the versions it's invaluable.
> > 
> > This removes intentional formatting that
> > makes reading comments easier.
> 
> And this matters at the end of the patch series because ?

It removes intentional formatting that makes
reading comments easier.

And diff -w still works just fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp

[Joe Perches]

On Sun, 2016-01-03 at 14:10 +, One Thousand Gnomes wrote:
> On Sat, 02 Jan 2016 23:54:28 -0800
> Joe Perches <j...@perches.com> wrote:
> 
> > On Sun, 2016-01-03 at 16:06 +1100, Finn Thain wrote:
> > > Hanging indentation was a poor choice for the text inside comments. It
> > > has been used in the wrong places and done badly elsewhere. There is
> > > little consistency within any file. One fork of the core driver uses
> > > tabs for this indentation while the other uses spaces. Better to use
> > > flush-left alignment throughout.
> > > 
> > > This patch is the result of the following substitution. It replaces tabs
> > > and spaces at the start of a comment line with a single space.
> > > 
> > > perl -i -pe 's,^(\t*[/ ]\*)[ \t]+,$1 ,' drivers/scsi/{atari_,}NCR5380.c 
> > > 
> > > This removes some unimportant discrepancies between the two core driver
> > > forks so that the important ones become obvious, to facilitate
> > > reunification.
> > 
> > I still think this patch is poor at best and
> > overall not useful.
> 
> I would beg to differ. As a tool for understanding the differences as you
> step through the versions it's invaluable.

This removes intentional formatting that
makes reading comments easier.

diff -w works well.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp

[Joe Perches]

On Sun, 2016-01-03 at 16:06 +1100, Finn Thain wrote:
> Hanging indentation was a poor choice for the text inside comments. It
> has been used in the wrong places and done badly elsewhere. There is
> little consistency within any file. One fork of the core driver uses
> tabs for this indentation while the other uses spaces. Better to use
> flush-left alignment throughout.
> 
> This patch is the result of the following substitution. It replaces tabs
> and spaces at the start of a comment line with a single space.
> 
> perl -i -pe 's,^(\t*[/ ]\*)[ \t]+,$1 ,' drivers/scsi/{atari_,}NCR5380.c 
> 
> This removes some unimportant discrepancies between the two core driver
> forks so that the important ones become obvious, to facilitate
> reunification.

I still think this patch is poor at best and
overall not useful.
@@ -32,15 +32,15 @@
>  /*
>   * Further development / testing that should be done :
>   * 1.  Cleanup the NCR5380_transfer_dma function and DMA operation complete
> - * code so that everything does the same thing that's done at the
> - * end of a pseudo-DMA read operation.
> + * code so that everything does the same thing that's done at the
> + * end of a pseudo-DMA read operation.
>   *
>   * 2.  Fix REAL_DMA (interrupt driven, polled works fine) -
> - * basically, transfer size needs to be reduced by one
> - * and the last byte read as is done with PSEUDO_DMA.
> + * basically, transfer size needs to be reduced by one
> + * and the last byte read as is done with PSEUDO_DMA.
>   *
>   * 4.  Test SCSI-II tagged queueing (I have no devices which support
> - *  tagged queueing)
> + * tagged queueing)

Numbering 1, 2, then 4 could be improved.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp

[Joe Perches]

On Wed, 2015-12-23 at 13:03 +1100, Finn Thain wrote:
> On Tue, 22 Dec 2015, Joe Perches wrote:
> 
> > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote:
> > > On Tue, 22 Dec 2015, Joe Perches wrote:
> > > 
> > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote:
> > > > > This patch is just the result of two substitutions. The first 
> > > > > removes any tabs and spaces at the end of the line. The second 
> > > > > replaces runs of tabs and spaces at the beginning of comment lines 
> > > > > with a single space.
> > > > 
> > > > I think the second of these isn't done well.
> > > 
> > > The aim of this patch is not to fix code style, but to make it 
> > > possible to compare these two files so that the fork can be repaired. 
> > > Regexp is very helpful in creating uniformity (and a minimal diff).
> > > 
> > > If this was a coding style issue, we would be discussing the use of 
> > > kernel-doc format for the affected comments, not whitespace.
> > > 
> > > > Many of these comments post reformatting are much worse to read 
> > > > because of lost alignment.
> > > 
> > > You exaggerate a very trivial point.
> > 
> > 
> > 
> > I prefer that all patches be improvements.
> > 
> 
> Agreed. But the example you cited is an improvement, in that it creates 
> consistency.

I think "consistency" isn't a useful argument.
The kernel code doesn't care about any other
external code bases.

> Like you, I prefer to see formal parameters aligned when wrapped. But this 
> isn't a formal parameter list, it is a comment, and no comment should 
> duplicate code.
> 
> Can you suggest a better regexp? Since this is patch 68 in the series, 
> there is a good chance that it will need to be regenerated.

I suggest you do 2 patches here.  One that removes
unnecessary trailing spaces and converts multiple
leading spaces to tabs where appropriate and a
second patch that fixes whatever odd indentation
that does exist after comment leading *.  I think
there aren't many instances of those and I think
those should be done by hand rather than regex.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp

[Joe Perches]

On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote:
> On Tue, 22 Dec 2015, Joe Perches wrote:
> 
> > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote:
> > > This patch is just the result of two substitutions. The first removes 
> > > any tabs and spaces at the end of the line. The second replaces runs 
> > > of tabs and spaces at the beginning of comment lines with a single 
> > > space.
> > 
> > I think the second of these isn't done well.
> 
> The aim of this patch is not to fix code style, but to make it possible to 
> compare these two files so that the fork can be repaired. Regexp is very 
> helpful in creating uniformity (and a minimal diff).
> 
> If this was a coding style issue, we would be discussing the use of 
> kernel-doc format for the affected comments, not whitespace.
> 
> > Many of these comments post reformatting are
> > much worse to read because of lost alignment.
> 
> You exaggerate a very trivial point.



I prefer that all patches be improvements.

> I admit that a small proportion of comments are slightly less readable. 
> But it is the diff that needs to be readable in order to resolve the fork.

diff -w works well.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp

[Joe Perches]

On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote:
> This patch is just the result of two substitutions. The first removes any
> tabs and spaces at the end of the line. The second replaces runs of
> tabs and spaces at the beginning of comment lines with a single space.

I think the second of these isn't done well.
Many of these comments post reformatting are
much worse to read because of lost alignment.

For instance:

> +/*
>   * Function : int NCR5380_select(struct Scsi_Host *instance,
> - *   struct scsi_cmnd *cmd)
> + * struct scsi_cmnd *cmd)
>   *
>   * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing command,
> - *  including ARBITRATION, SELECTION, and initial message out for 
> - *  IDENTIFY and queue messages. 
> + * including ARBITRATION, SELECTION, and initial message out for
> + * IDENTIFY and queue messages.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [SCSI] osd: fix signed char versus %02x issue

[Joe Perches]

On Thu, 2015-12-10 at 14:13 -0500, Martin K. Petersen wrote:
> > > > > > "Andy" == Andy Shevchenko  writes:
> 
> Andy> I have several patches on SCSI subsytem like this one. Some of
> Andy> them didn't manage kernel (even having Ack!) for years already.
> Andy> Is it okay if I collect them together and send a bunch once again
> 
> Re-sending to linux-scsi is fine. The trick is finding people willing to
> review them...

Generally speaking, it hasn't been for lack of review.

SCSI has been one of the slowest subsystems to apply
simple defect correction (not whitespace style) patches.

Mostly these patches have been ignored.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] storvsc: add more logging for error and warning messages

[Joe Perches]

On Thu, 2015-12-03 at 19:47 -0800, Long Li wrote:
> Introduce a logging level for storvsc to log certain error/warning
> messages. Those messages are helpful in some environments, e.g.
> Microsoft Azure, for customer support and troubleshooting purposes.
[]
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
[]
> +static inline bool do_logging(int level)
> +{
> + return (logging_level >= level) ? true : false;

The ternary is not necessary

return logging_level >= level;

is enough

> +}
> +
> +
>  struct vmscsi_win8_extension {
>   /*
>    * The following were added in Windows 8
> @@ -1183,7 +1198,7 @@ static void storvsc_command_completion(struct 
> storvsc_cmd_request *cmd_request)
>  
>   scmnd->result = vm_srb->scsi_status;
>  
> - if (scmnd->result) {
> + if (scmnd->result && do_logging(STORVSC_LOGGING_ERROR)) {
>   if (scsi_normalize_sense(scmnd->sense_buffer,
>   SCSI_SENSE_BUFFERSIZE, _hdr))
>   scsi_print_sense_hdr(scmnd->device, "storvsc",

Is it appropriate to make this scsi_normalize_sense call
conditional on do_logging here?

> @@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct hv_device 
> *device,
>   stor_pkt->vm_srb.sense_info_length =
>   vstor_packet->vm_srb.sense_info_length;
>  
> + if (vstor_packet->vm_srb.scsi_status != 0 ||
> + vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
> + if (do_logging(STORVSC_LOGGING_WARN))
> + dev_warn(>device,
> + "cmd 0x%x scsi status 0x%x srb status 0x%x\n",
> + stor_pkt->vm_srb.cdb[0],
> + vstor_packet->vm_srb.scsi_status,
> + vstor_packet->vm_srb.srb_status);

It might make some sense to use another macro indirection like

#define svc_log_warn(dev, level, fmt, ...)  \
do {\
if (do_logging(STORSVC_LOGGING_##level) \
dev_warn(&(dev)->device, fmt, ##__VA_ARGS__);   \
} while (0)

So a use could be:

if (vstore_packet...)
svc_log_warn(device, WARN, ...);

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] arcmsr: Split dma resource allocation to a new function

[Joe Perches]

On Thu, 2015-11-26 at 19:41 +0800, Ching Huang wrote:
> split dma resource allocation and io register assignment from get_config to a 
> new function arcmsr_alloc_io_queue.

trivia:

> diff -uprN a/drivers/scsi/arcmsr/arcmsr_hba.c 
> b/drivers/scsi/arcmsr/arcmsr_hba.c
[]
> +static bool arcmsr_alloc_io_queue(struct AdapterControlBlock *acb)
> +{
[]
> + dma_coherent = dma_alloc_coherent(>dev, 
> acb->roundup_ccbsize,
> + _coherent_handle, GFP_KERNEL);
> + if (!dma_coherent){
> + pr_notice("arcmsr%d: DMA allocation failed.\n", 
> acb->host->host_no);
> + return false;
> + }
> + memset(dma_coherent, 0, acb->roundup_ccbsize);
> 

There is a dma_zalloc_coherent

(and even more trivially)

Most all of your error messages don't use periods.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] arcmsr: adds code for support areca new adapter ARC1203

[Joe Perches]

On Tue, 2015-11-24 at 16:17 +0800, Ching Huang wrote:
> From: Ching Huang 
> 
> Support areca new PCIe to SATA RAID adapter ARC1203

Why add the dma_free_coherent to an old data path?
Is that a general bug fix that should be backported?

btw: the printk above the dma_free_coherent use
 shouldn't use a \ line continuation.
 it adds undesired whitespace to the format.

> diff -uprN a/drivers/scsi/arcmsr/arcmsr_hba.c 
> b/drivers/scsi/arcmsr/arcmsr_hba.c
[]
> @@ -2660,10 +2670,19 @@ static bool arcmsr_hbaB_get_config(struc
>   iop_firm_version = (char __iomem *)(®->message_rwbuffer[17]);>  > 
> /*firm_version,17,68-83*/
>   iop_device_map = (char __iomem *)(®->message_rwbuffer[21]);>> 
> /*firm_version,21,84-99*/
>  
> + arcmsr_wait_firmware_ready(acb);
> + writel(ARCMSR_MESSAGE_START_DRIVER_MODE, reg->drv2iop_doorbell);
> + if (!arcmsr_hbaB_wait_msgint_ready(acb)) {
> + printk(KERN_ERR "arcmsr%d: can't set driver mode.\n", 
> acb->host->host_no);
> + goto gcfg1;
> + }
>   writel(ARCMSR_MESSAGE_GET_CONFIG, reg->drv2iop_doorbell);
>   if (!arcmsr_hbaB_wait_msgint_ready(acb)) {
>   printk(KERN_NOTICE "arcmsr%d: wait 'get adapter firmware \
>   miscellaneous data' timeout \n", acb->host->host_no);
> +gcfg1:
> + dma_free_coherent(>pdev->dev, acb->roundup_ccbsize,
> + acb->dma_coherent2, acb->dma_coherent_handle2);
>   return false;
>   }
>   count = 8;

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] arcmsr: adds code for support areca new adapter ARC1203

[Joe Perches]

On Tue, 2015-11-24 at 17:53 +0800, Ching Huang wrote:
> On Tue, 2015-11-24 at 01:33 -0800, Joe Perches wrote:
> > On Tue, 2015-11-24 at 16:17 +0800, Ching Huang wrote:
> > > From: Ching Huang <ching2...@areca.com.tw>
> > > 
> > > Support areca new PCIe to SATA RAID adapter ARC1203
> > 
> > Why add the dma_free_coherent to an old data path?
> > Is that a general bug fix that should be backported?
> 
> That's right. It's need to release the allocated resource for failed
> condition.

Then the dma_free_coherent addition should be a separate patch.

Style trivia:

The goto to another error path like that is odd and
the label is unintelligible.

Ideally error condition handling would use a goto and
a separate and obviously named label.  Use multiple
labels for cases with more complicated unwinding.

Dan Carpenter has written about this several times.

For this use, something like:

writel(ARCMSR_MESSAGE_START_DRIVER_MODE, reg->drv2iop_doorbell);
if (!arcmsr_hbaB_wait_msgint_ready(acb)) {
logging_message(...);
goto
err_free_resource;
}
writel(ARCMSR_MESSAGE_GET_CONFIG, reg->drv2iop_doorbell);
if (!arcmsr_hbaB_wait_msgint_ready(acb)) {
logging_message(...);
goto err_free_resource;
}

[success path...]

return true;

err_free_resource:
dma_free_coherent(...);

return false;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] megaraid_sas: Convert dev_printk to dev_

[Joe Perches]

On Tue, 2015-10-27 at 16:26 +0800, Weidong Wang wrote:
> Reduce object size a little by using dev_
> calls instead of dev_printk(KERN_.

This is also not the same output.

dev_printk(KERN_DEBUG vs dev_dbg has the same
behavior as printk(KERN_DEBUG vs pr_debug

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c 
> b/drivers/scsi/megaraid/megaraid_sas_base.c
[]
> @@ -1884,7 +1884,7 @@ static int megasas_get_ld_vf_affiliation_111(struct 
> megasas_instance *instance,
>   cmd = megasas_get_cmd(instance);
>  
>   if (!cmd) {
> - dev_printk(KERN_DEBUG, >pdev->dev, 
> "megasas_get_ld_vf_affiliation_111:"
> + dev_dbg(>pdev->dev, 
> "megasas_get_ld_vf_affiliation_111:"
>  "Failed to get cmd for scsi%d\n",
>   instance->host->host_no);
>   return -ENOMEM;
[]
> @@ -5243,7 +5243,7 @@ static int megasas_probe_one(struct pci_dev *pdev,
>>consumer_h);
>  
>   if (!instance->producer || !instance->consumer) {
> - dev_printk(KERN_DEBUG, >dev, "Failed to allocate"
> + dev_dbg(>dev, "Failed to allocate"
>  "memory for producer, consumer\n");

Note the lack of a space between coalesced string segment words.
That's one of the reasons to coalesce them.


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] megaraid_sas: Convert printk to printk_

[Joe Perches]

On Tue, 2015-10-27 at 16:26 +0800, Weidong Wang wrote:
> Reduce object size a little by using pr_
> calls instead of printk(KERN_.

pr_debug does not behave the same as printk(KERN_DEBUG

pr_debug is only active when DEBUG is #defined or dynamic_debug
is enabled.

printk(KERN_DEBUG is always emitted as long as the debug level
is enabled for the console.

At a minimum, your commit message should show you know that.

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c 
> b/drivers/scsi/megaraid/megaraid_sas_base.c
[]
> @@ -5889,7 +5889,7 @@ static int megasas_mgmt_fasync(int fd, struct file 
> *filep, int mode)
>   return 0;
>   }
>  
> - printk(KERN_DEBUG "megasas: fasync_helper failed [%d]\n", rc);
> + pr_debug("megasas: fasync_helper failed [%d]\n", rc);

[]

> @@ -6233,7 +6233,7 @@ static int megasas_mgmt_ioctl_aen(struct file *file, 
> unsigned long arg)
>   u32 wait_time = MEGASAS_RESET_WAIT_TIME;
>  
>   if (file->private_data != file) {
> - printk(KERN_DEBUG "megasas: fasync_helper was not "
> + pr_debug("megasas: fasync_helper was not "
>  "called first\n");

Please also coalesce format strings where possible.


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi: pmcraid: trivial typo in printk

[Joe Perches]

Trivial typo fixes:

o \b should be \n
o coalesce format to avoid excess spaces

Signed-off-by: Joe Perches j...@perches.com
---
And another here:

 drivers/scsi/pmcraid.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index ed31d8c..b421de2 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -1558,8 +1558,7 @@ static void pmcraid_handle_config_change(struct 
pmcraid_instance *pinstance)
cfg_entry = ccn_hcam-cfg_entry;
fw_version = be16_to_cpu(pinstance-inq_data-fw_version);
 
-   pmcraid_info(CCN(%x): %x timestamp: %llx type: %x lost: %x flags: %x \
-res: %x:%x:%x:%x\n,
+   pmcraid_info(CCN(%x): %x timestamp: %llx type: %x lost: %x flags: %x 
res: %x:%x:%x:%x\n,
 pinstance-ccn.hcam-ilid,
 pinstance-ccn.hcam-op_code,
((pinstance-ccn.hcam-timestamp1) |
@@ -1583,7 +1582,7 @@ static void pmcraid_handle_config_change(struct 
pmcraid_instance *pinstance)
if (pinstance-ccn.hcam-notification_lost) {
cfgcmd = pmcraid_get_free_cmd(pinstance);
if (cfgcmd) {
-   pmcraid_info(lost CCN, reading config table\b);
+   pmcraid_info(lost CCN, reading config table\n);
pinstance-reinit_cfg_table = 1;
pmcraid_querycfg(cfgcmd);
} else {


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] megaraid : use dev_printk when possible

[Joe Perches]

On Mon, 2015-06-01 at 10:40 -0500, Bjorn Helgaas wrote:
 Use dev_printk() when possible to make messages more useful.
[]
 diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
[]
 @@ -268,7 +268,7 @@ mega_query_adapter(adapter_t *adapter)
   raw_mbox[2] = NC_SUBOP_PRODUCT_INFO;/* i.e. 0x0E */
  
   if ((retval = issue_scb_block(adapter, raw_mbox)))
 - printk(KERN_WARNING
 + dev_warn(adapter-dev-dev,
   megaraid: Product_info cmd failed with error: %d\n,
   retval);

Wouldn't these be a bit redundant with a megaraid:  prefix now?
Perhaps it'd be nicer to realign arguments too.


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] megaraid : use dev_printk when possible

[Joe Perches]

On Mon, 2015-06-01 at 11:40 -0500, Bjorn Helgaas wrote:
 On Mon, Jun 1, 2015 at 11:10 AM, Joe Perches j...@perches.com wrote:
  On Mon, 2015-06-01 at 10:40 -0500, Bjorn Helgaas wrote:
  Use dev_printk() when possible to make messages more useful.
  []
  diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
  []
  @@ -268,7 +268,7 @@ mega_query_adapter(adapter_t *adapter)
raw_mbox[2] = NC_SUBOP_PRODUCT_INFO;/* i.e. 0x0E */
 
if ((retval = issue_scb_block(adapter, raw_mbox)))
  - printk(KERN_WARNING
  + dev_warn(adapter-dev-dev,
megaraid: Product_info cmd failed with error: %d\n,
retval);
 
  Wouldn't these be a bit redundant with a megaraid:  prefix now?
  Perhaps it'd be nicer to realign arguments too.
 
 Yes, you're right.  I took out most of the megaraid:  prefixes, but
 I missed a few.
 
 I tried not to realign things too much because it's hard to know where
 to stop, and I don't want to spend a week on this.

;)  I try to realign all modified statements.

 I'll post a v3 in a day or two in case there are other comments.

Thanks.


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/12] treewide: Use DECLARE_BITMAP

[Joe Perches]

Use the standard method to declare a bitmap array.

Joe Perches (12):
  ARM: mach-imx: iomux-imx31: Use DECLARE_BITMAP
  dmaengine: rcar-dmac: Use DECLARE_BITMAP
  drm/amdkfd: Use DECLARE_BITMAP
  drm/radeon: Use DECLARE_BITMAP
  IB/ehca: Use DECLARE_BITMAP
  bcache: Use DECLARE_BITMAP
  spider_net: Use DECLARE_BITMAP
  s390/sclp: Use DECLARE_BITMAP
  [SCSI] qla4xxx: Use DECLARE_BITMAP
  scsi: Use DECLARE_BITMAP
  logfs: Use DECLARE_BITMAP
  sunrpc: Use DECLARE_BITMAP

 arch/arm/mach-imx/iomux-imx31.c   | 2 +-
 drivers/dma/sh/rcar-dmac.c| 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 5 ++---
 drivers/gpu/drm/radeon/radeon.h   | 2 +-
 drivers/infiniband/hw/ehca/ipz_pt_fn.h| 2 +-
 drivers/md/bcache/journal.c   | 2 +-
 drivers/net/ethernet/toshiba/spider_net.c | 3 +--
 drivers/s390/char/sclp_cmd.c  | 2 +-
 drivers/scsi/qla4xxx/ql4_def.h| 2 +-
 drivers/scsi/sr.c | 2 +-
 fs/logfs/logfs.h  | 2 +-
 net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
 12 files changed, 13 insertions(+), 15 deletions(-)

-- 
2.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/25] csiostor: Use bool function return values of true/false not 1/0

[Joe Perches]

Use the normal return values for bool functions

Signed-off-by: Joe Perches j...@perches.com
---
 drivers/scsi/csiostor/csio_scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/csiostor/csio_scsi.c 
b/drivers/scsi/csiostor/csio_scsi.c
index 2c4562d..5754fb6 100644
--- a/drivers/scsi/csiostor/csio_scsi.c
+++ b/drivers/scsi/csiostor/csio_scsi.c
@@ -147,9 +147,9 @@ csio_scsi_itnexus_loss_error(uint16_t error)
case FW_ERR_RDEV_LOST:
case FW_ERR_RDEV_LOGO:
case FW_ERR_RDEV_IMPL_LOGO:
-   return 1;
+   return true;
}
-   return 0;
+   return false;
 }
 
 /*
-- 
2.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/25] treewide: Use bool function return values of true/false not 1/0

[Joe Perches]

Joe Perches (25):
  arm: Use bool function return values of true/false not 1/0
  arm64: Use bool function return values of true/false not 1/0
  hexagon: Use bool function return values of true/false not 1/0
  ia64: Use bool function return values of true/false not 1/0
  mips: Use bool function return values of true/false not 1/0
  powerpc: Use bool function return values of true/false not 1/0
  s390: Use bool function return values of true/false not 1/0
  sparc: Use bool function return values of true/false not 1/0
  tile: Use bool function return values of true/false not 1/0
  unicore32: Use bool function return values of true/false not 1/0
  x86: Use bool function return values of true/false not 1/0
  virtio_console: Use bool function return values of true/false not 1/0
  csiostor: Use bool function return values of true/false not 1/0
  dcache: Use bool function return values of true/false not 1/0
  nfsd: nfs4state: Use bool function return values of true/false not 1/0
  include/linux: Use bool function return values of true/false not 1/0
  sound: Use bool function return values of true/false not 1/0
  rcu: tree_plugin: Use bool function return values of true/false not 1/0
  sched: Use bool function return values of true/false not 1/0
  ftrace: Use bool function return values of true/false not 1/0
  slub: Use bool function return values of true/false not 1/0
  bridge: Use bool function return values of true/false not 1/0
  netfilter: Use bool function return values of true/false not 1/0
  security: Use bool function return values of true/false not 1/0
  sound: wm5100-tables: Use bool function return values of true/false not 1/0

 arch/arm/include/asm/dma-mapping.h   |  8 ++--
 arch/arm/include/asm/kvm_emulate.h   |  2 +-
 arch/arm/mach-omap2/powerdomain.c| 14 +++---
 arch/arm64/include/asm/dma-mapping.h |  2 +-
 arch/hexagon/include/asm/dma-mapping.h   |  2 +-
 arch/ia64/include/asm/dma-mapping.h  |  2 +-
 arch/mips/include/asm/dma-mapping.h  |  2 +-
 arch/powerpc/include/asm/dcr-native.h|  2 +-
 arch/powerpc/include/asm/dma-mapping.h   |  4 +-
 arch/powerpc/include/asm/kvm_book3s_64.h |  4 +-
 arch/powerpc/sysdev/dcr.c|  2 +-
 arch/s390/include/asm/dma-mapping.h  |  2 +-
 arch/sparc/mm/init_64.c  |  8 ++--
 arch/tile/include/asm/dma-mapping.h  |  2 +-
 arch/unicore32/include/asm/dma-mapping.h |  2 +-
 arch/x86/include/asm/archrandom.h|  2 +-
 arch/x86/include/asm/dma-mapping.h   |  2 +-
 arch/x86/include/asm/kvm_para.h  |  2 +-
 arch/x86/kvm/cpuid.h |  2 +-
 arch/x86/kvm/vmx.c   | 72 ++--
 drivers/char/virtio_console.c|  2 +-
 drivers/scsi/csiostor/csio_scsi.c|  4 +-
 fs/dcache.c  | 12 ++---
 fs/nfsd/nfs4state.c  |  2 +-
 include/linux/blkdev.h   |  2 +-
 include/linux/ide.h  |  2 +-
 include/linux/kgdb.h |  2 +-
 include/linux/mfd/db8500-prcmu.h |  2 +-
 include/linux/mm.h   |  2 +-
 include/linux/power_supply.h |  8 ++--
 include/linux/ssb/ssb_driver_extif.h |  2 +-
 include/linux/ssb/ssb_driver_gige.h  | 16 +++
 include/sound/soc.h  |  4 +-
 kernel/rcu/tree_plugin.h |  4 +-
 kernel/sched/auto_group.h|  2 +-
 kernel/sched/completion.c| 16 ---
 kernel/trace/ftrace.c| 10 ++--
 mm/slub.c| 12 ++---
 net/bridge/br_private.h  |  2 +-
 net/ipv4/netfilter/ipt_ah.c  |  2 +-
 net/netfilter/ipset/ip_set_hash_ip.c |  8 ++--
 net/netfilter/ipset/ip_set_hash_ipmark.c |  8 ++--
 net/netfilter/ipset/ip_set_hash_ipport.c |  8 ++--
 net/netfilter/ipset/ip_set_hash_ipportip.c   |  8 ++--
 net/netfilter/ipset/ip_set_hash_ipportnet.c  |  8 ++--
 net/netfilter/ipset/ip_set_hash_net.c|  8 ++--
 net/netfilter/ipset/ip_set_hash_netiface.c   |  8 ++--
 net/netfilter/ipset/ip_set_hash_netport.c|  8 ++--
 net/netfilter/ipset/ip_set_hash_netportnet.c |  8 ++--
 net/netfilter/xt_connlimit.c |  2 +-
 net/netfilter/xt_hashlimit.c |  2 +-
 net/netfilter/xt_ipcomp.c|  2 +-
 security/apparmor/file.c |  8 ++--
 security/apparmor/policy.c   | 10 ++--
 sound/soc/codecs/wm5100-tables.c | 12 ++---
 55 files changed, 178 insertions(+), 176 deletions(-)

-- 
2.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/25] treewide: Use bool function return values of true/false not 1/0

[Joe Perches]

On Mon, 2015-03-30 at 17:07 -0700, Casey Schaufler wrote:
 On 3/30/2015 4:45 PM, Joe Perches wrote:
  Joe Perches (25):
arm: Use bool function return values of true/false not 1/0

[etc...]

 Why, and why these in particular?

bool functions are probably better returning
bool values instead of 1 and 0.

Especially when the functions intermix returning
returning 1/0 and true/false.

(there are only a couple of those though)

These are all the remaining instances in the
kernel tree.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RESEND] aic7xxx: replace kmalloc/memset by kzalloc

[Joe Perches]

On Tue, 2015-03-24 at 13:46 -0700, Michael Opdenacker wrote:
 Hi,
 
 On 03/22/2015 11:59 PM, Hannes Reinecke wrote:
  On 03/22/2015 05:31 PM, Michael Opdenacker wrote:
  This replaces kmalloc + memset by a call to kzalloc
  (or kcalloc when appropriate, which zeroes memory too)
 
  This also fixes one checkpatch.pl issue in the process.
 
  This improvement was suggested by make coccicheck
 
  Signed-off-by: Michael Opdenacker michael.opdenac...@free-electrons.com
  Reviewed-by: Hannes Reinecke h...@suse.de
 
 I'm sending a version that reverts the use of kcalloc() instead of
 kzalloc(). For reasons I don't understand, I didn't see the end of
 Robert Elliott's comment that the use of kcalloc() could prevent the
 compiler from detecting an overflow.

I'm confused.  I don't see that comment either, but
the entire point of kcalloc is to prevent overflows
by returning NULL when an overflow might occur.

from include/linux/slab.h:

/**
 * kmalloc_array - allocate memory for an array.
 * @n: number of elements.
 * @size: element size.
 * @flags: the type of memory to allocate (see kmalloc).
 */
static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)
{
if (size != 0  n  SIZE_MAX / size)
return NULL;
return __kmalloc(n * size, flags);
}

/**
 * kcalloc - allocate memory for an array. The memory is set to zero.
 * @n: number of elements.
 * @size: element size.
 * @flags: the type of memory to allocate (see kmalloc).
 */
static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
{
return kmalloc_array(n, size, flags | __GFP_ZERO);
}


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xen-scsiback: use DRV_PFX in the pr macros and DPRINTK

[Joe Perches]

On Fri, 2015-03-06 at 17:33 +0800, Chentao (Boby) wrote:
 #ifdef pr_fmt
 #undef pr_fmt
 #endif
 #define pr_fmt(fmt) xen-pvscsi:  fmt

No, just use add
#define pr_fmt(fmt) xen-pvscsi:  fmt
before the first #include.
The #ifdef/#undef/#endif isn't necessary.

 Then replace all DPRINTK with pr_debug.

Yes on this one.



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xen-scsiback: use DRV_PFX in the pr macros and DPRINTK

[Joe Perches]

On Wed, 2015-03-04 at 18:32 +, Tao Chen wrote:
 Defined the string of {xen-pvscsi: } as DRV_PFX, then use it in the pr 
 sentences and DPRINTK.
 Also fixed up some comments just as eliminate redundant white spaces and 
 format the code.
 These will make the code easier to read.

It'd probaby be better just to use pr_fmt
before any include and remove all the DRV_PRV uses

#define pr_fmt(fmt) xen-pvscsi:  fmt

 diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
[]
 @@ -69,8 +69,10 @@
  #include xen/interface/grant_table.h
  #include xen/interface/io/vscsiif.h
  
 +#define DRV_PFX xen-pvscsi: 
  #define DPRINTK(_f, _a...)   \
 - pr_debug((file=%s, line=%d)  _f, __FILE__ , __LINE__ , ## _a)
 + pr_debug(DRV_PFX (file=%s, line=%d)  _f,  \
 + __FILE__ , __LINE__ , ## _a)

I'd also remove DPRINTK and just use pr_debug directly
as dynamic_debug can emit file and line as desired.

 @@ -84,7 +86,7 @@ struct ids_tuple {
  
  struct v2p_entry {
   struct ids_tuple v; /* translate from */
 - struct scsiback_tpg *tpg;   /* translate to   */
 + struct scsiback_tpg *tpg;   /* translate to */

superfluous change.


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: chelsio: Use a more common const struct pci_device_id foo[] style

[Joe Perches]

On Mon, 2015-02-16 at 19:07 +, Casey Leedom wrote:
   I understand that OS-independence issues aren't something which are
 normally accommodated, but as long as definitions don't introduce
 unnecessary foreign intrusion I would hope that it would be okay.
 As I noted, our t4_regs.h file is also OS-independent and used by six
 other OS device drivers.  Putting Linux definitions into
 t4_pci_id_tbl.h would be somewhat akin to injecting Linux dependencies
 into t4_regs.h.
 
   But, if the change must be made, then we'll just maintain a
 translation between our Common Code and the kernel.org code.  If
 that's the case, probably the best documentation for the proposed
 CH_PCI_ID_TABLE_ENTRY_DATA might be something like:
 
  * CH_PCI_ID_TABLE_ENTRY_DATA
  *   -- Used for the individual PCI Device ID entries for the
 PCI_VDEVICE() dev
  *   -- parameter.
 
   So it sounds like Chelsio would be required to make this change
 then?  I'm still unclear on the likes of responsibility/authority
 here.  We're being told that we must do this but we have to be the
 ones requesting it?  Sorry for my confusion.

It's just a suggested patch.
It's your code, you don't _have_ to do anything.

 (Which is doubly apparent since I came into work this morning only to
 realize that it's a company holiday.  Color me a moron today.)

Yay for holidays.

It can be personally beneficial to take the day off.
It could also be very productive to work with no distractions.

As always, your choice...

cheers, Joe


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

chelsio: Use a more common const struct pci_device_id foo[] style

[Joe Perches]

Chelsio code shares a pci_device_table from an #include file.
Make the include guard simpler and make the arrays const.

Reduces data by moving tables to text.

Removed unnecessary macros:
o CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
o CH_PCI_DEVICE_ID_TABLE_DEFINE_END
o CH_PCI_ID_TABLE_ENTRY (moved to the .h file)
Added new macro define:
o CH_PCI_ID_TABLE_ENTRY_DATA

  text data bss dec hex filename
  50550 923 172   51645c9bd 
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.new
  469354531 172   51638c9b6 
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o.old
  27864 355   8   282276e43 
drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.new
  260722203   8   282836e7b 
drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.o.old
   9734 450  24   1020827e0 drivers/scsi/csiostor/csio_init.o.new
   79422242  24   1020827e0 drivers/scsi/csiostor/csio_init.o.old

Signed-off-by: Joe Perches j...@perches.com
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c| 17 --
 drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h | 38 --
 .../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c| 10 ++
 drivers/scsi/csiostor/csio_init.c  | 11 +++
 4 files changed, 27 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index a22cf93..8a01eeb 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -123,23 +123,18 @@ struct filter_entry {
 
 /* Macros needed to support the PCI Device ID Table ...
  */
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN \
-   static struct pci_device_id cxgb4_pci_tbl[] = {
-#define CH_PCI_DEVICE_ID_FUNCTION 0x4
+
+#define CH_PCI_DEVICE_ID_FUNCTION  0x4
+#define CH_PCI_ID_TABLE_ENTRY_DATA 4
 
 /* Include PCI Device IDs for both PF4 and PF0-3 so our PCI probe() routine is
  * called for both.
  */
-#define CH_PCI_DEVICE_ID_FUNCTION2 0x0
-
-#define CH_PCI_ID_TABLE_ENTRY(devid) \
-   {PCI_VDEVICE(CHELSIO, (devid)), 4}
-
-#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END \
-   { 0, } \
-   }
+#define CH_PCI_DEVICE_ID_FUNCTION2 0x0
 
+static const struct pci_device_id cxgb4_pci_tbl[] = {
 #include t4_pci_id_tbl.h
+};
 
 #define FW4_FNAME cxgb4/t4fw.bin
 #define FW5_FNAME cxgb4/t5fw.bin
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h 
b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
index ddfb5b8..f648091 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
@@ -39,9 +39,6 @@
  *
  * The macros are:
  *
- * CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
- *   -- Used to start the definition of the PCI ID Table.
- *
  * CH_PCI_DEVICE_ID_FUNCTION
  *   -- The PCI Function Number to use in the PCI Device ID Table.  0
  *   -- for drivers attaching to PF0-3, 4 for drivers attaching to PF4,
@@ -51,25 +48,17 @@
  *   -- If defined, create a PCI Device ID Table with both
  *   -- CH_PCI_DEVICE_ID_FUNCTION and CH_PCI_DEVICE_ID_FUNCTION2 populated.
  *
- * CH_PCI_ID_TABLE_ENTRY(DeviceID)
+ * CH_PCI_ID_TABLE_ENTRY_DATA(DeviceID)
  *   -- Used for the individual PCI Device ID entries.  Note that we will
  *   -- be adding a trailing comma (,) after all of the entries (and
  *   -- between the pairs of entries if CH_PCI_DEVICE_ID_FUNCTION2 is defined).
- *
- * CH_PCI_DEVICE_ID_TABLE_DEFINE_END
- *   -- Used to finish the definition of the PCI ID Table.  Note that we
- *   -- will be adding a trailing semi-colon (;) here.
  */
-#ifdef CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
 
 #ifndef CH_PCI_DEVICE_ID_FUNCTION
 #error CH_PCI_DEVICE_ID_FUNCTION not defined!
 #endif
-#ifndef CH_PCI_ID_TABLE_ENTRY
-#error CH_PCI_ID_TABLE_ENTRY not defined!
-#endif
-#ifndef CH_PCI_DEVICE_ID_TABLE_DEFINE_END
-#error CH_PCI_DEVICE_ID_TABLE_DEFINE_END not defined!
+#ifndef CH_PCI_ID_TABLE_ENTRY_DATA
+#error CH_PCI_ID_TABLE_ENTRY_DATA not defined!
 #endif
 
 /* T4 and later ASICs use a PCI Device ID scheme of 0xVFPP where:
@@ -81,19 +70,22 @@
  * We use this consistency in order to create the proper PCI Device IDs
  * for the specified CH_PCI_DEVICE_ID_FUNCTION.
  */
+
+#define CH_PCI_ID_TABLE_ENTRY(devid)   \
+   { PCI_VDEVICE(CHELSIO, devid), CH_PCI_ID_TABLE_ENTRY_DATA }
+
 #ifndef CH_PCI_DEVICE_ID_FUNCTION2
-#define CH_PCI_ID_TABLE_FENTRY(devid) \
-   CH_PCI_ID_TABLE_ENTRY((devid) | \
+#define CH_PCI_ID_TABLE_FENTRY(devid)  \
+   CH_PCI_ID_TABLE_ENTRY((devid) | \
  ((CH_PCI_DEVICE_ID_FUNCTION)  8))
 #else
-#define CH_PCI_ID_TABLE_FENTRY(devid) \
-   CH_PCI_ID_TABLE_ENTRY((devid) | \
- ((CH_PCI_DEVICE_ID_FUNCTION)  8)), \
-   CH_PCI_ID_TABLE_ENTRY((devid) | \
+#define CH_PCI_ID_TABLE_FENTRY

Re: [PATCH] scsi: megaraid_sas: Prevent future %p disaster

[Joe Perches]

On Tue, 2015-02-10 at 22:16 +0530, Kashyap Desai wrote:
 Acked-by: Kashyap Desai kashyap.de...@avagotech.com
 
  -Original Message-
  From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
  Sent: Friday, February 06, 2015 8:04 PM
[]
  There is currently no %po format extension, so currently the letters
 on are
  simply skipped and the pointer is printed as expected (while missing the
 word
  on). However, it is probably only a matter of time before someone comes
 up
  with a %po extension, at which point this is likely to fail
 spectacularly.

Nice tool Rasmus

  diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
[]
  @@ -3105,7 +3105,7 @@ megasas_internal_reset_defer_cmds(struct
  megasas_instance *instance)
  for (i = 0; i  max_cmd; i++) {
  cmd = instance-cmd_list[i];
  if (cmd-sync_cmd == 1 || cmd-scmd) {
  -   printk(KERN_NOTICE megasas: moving
  cmd[%d]:%p:%d:%p
  +   printk(KERN_NOTICE megasas: moving
  cmd[%d]:%p:%d:%p 
  on the defer queue as
 internal\n,
  defer_index, cmd, cmd-sync_cmd, cmd-
  scmd);
 

It'd probably be nicer to coalesce the format.

printk(KERN_NOTICE megasas: moving cmd[%d]:%p:%d:%p on 
the defer queue as internal\n,
defer_ndex, cmd, cmd-sync, cmd-scmd);


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: megaraid_sas: Prevent future %p disaster

[Joe Perches]

Convert printks to pr_level

Miscellaneous:

o Coalesce formats
  Add and remove spaces where appropriate when coalescing
o Add pr_fmt
o Remove embedded prefixes
o Convert embedded function names to %s:, __func__
o Realign arguments
o Outdent one block that was inappropriately indented
o Use pr_cont where appropriate

Signed-off-by: Joe Perches j...@perches.com
---

Maybe this patch instead:

 drivers/scsi/megaraid/megaraid_sas_base.c   | 484 
 drivers/scsi/megaraid/megaraid_sas_fp.c |  13 +-
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 181 ---
 3 files changed, 291 insertions(+), 387 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c 
b/drivers/scsi/megaraid/megaraid_sas_base.c
index 890637f..263b209 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -31,6 +31,8 @@
  *  San Jose, California 95131
  */
 
+#define pr_fmt(fmt) megasas:  fmt
+
 #include linux/kernel.h
 #include linux/types.h
 #include linux/pci.h
@@ -217,7 +219,7 @@ struct megasas_cmd *megasas_get_cmd(struct megasas_instance
list_del_init(cmd-list);
atomic_set(cmd-mfi_mpt_pthr, MFI_MPT_DETACHED);
} else {
-   printk(KERN_ERR megasas: Command pool empty!\n);
+   pr_err(Command pool empty!\n);
}
 
spin_unlock_irqrestore(instance-mfi_pool_lock, flags);
@@ -382,9 +384,9 @@ megasas_adp_reset_xscale(struct megasas_instance *instance,
msleep(1000); /* sleep for 3 secs */
pcidata  = 0;
pci_read_config_dword(instance-pdev, MFI_1068_PCSR_OFFSET, pcidata);
-   printk(KERN_NOTICE pcidata = %x\n, pcidata);
+   pr_notice(pcidata = %x\n, pcidata);
if (pcidata  0x2) {
-   printk(KERN_NOTICE mfi 1068 offset read=%x\n, pcidata);
+   pr_notice(mfi 1068 offset read=%x\n, pcidata);
pcidata = ~0x2;
pci_write_config_dword(instance-pdev,
MFI_1068_PCSR_OFFSET, pcidata);
@@ -395,9 +397,9 @@ megasas_adp_reset_xscale(struct megasas_instance *instance,
pcidata  = 0;
pci_read_config_dword(instance-pdev,
MFI_1068_FW_HANDSHAKE_OFFSET, pcidata);
-   printk(KERN_NOTICE 1068 offset handshake read=%x\n, pcidata);
+   pr_notice(1068 offset handshake read=%x\n, pcidata);
if ((pcidata  0x) == MFI_1068_FW_READY) {
-   printk(KERN_NOTICE 1068 offset pcidt=%x\n, pcidata);
+   pr_notice(1068 offset pcidt=%x\n, pcidata);
pcidata = 0;
pci_write_config_dword(instance-pdev,
MFI_1068_FW_HANDSHAKE_OFFSET, pcidata);
@@ -836,15 +838,15 @@ megasas_adp_reset_gen2(struct megasas_instance *instance,
while ( !( HostDiag  DIAG_WRITE_ENABLE) ) {
msleep(100);
HostDiag = (u32)readl(hostdiag_offset);
-   printk(KERN_NOTICE RESETGEN2: retry=%x, hostdiag=%x\n,
-   retry, HostDiag);
+   pr_notice(RESETGEN2: retry=%x, hostdiag=%x\n,
+ retry, HostDiag);
 
if (retry++ = 100)
return 1;
 
}
 
-   printk(KERN_NOTICE ADP_RESET_GEN2: HostDiag=%x\n, HostDiag);
+   pr_notice(ADP_RESET_GEN2: HostDiag=%x\n, HostDiag);
 
writel((HostDiag | DIAG_RESET_ADAPTER), hostdiag_offset);
 
@@ -854,8 +856,8 @@ megasas_adp_reset_gen2(struct megasas_instance *instance,
while ( ( HostDiag  DIAG_RESET_ADAPTER) ) {
msleep(100);
HostDiag = (u32)readl(hostdiag_offset);
-   printk(KERN_NOTICE RESET_GEN2: retry=%x, hostdiag=%x\n,
-   retry, HostDiag);
+   pr_notice(RESET_GEN2: retry=%x, hostdiag=%x\n,
+ retry, HostDiag);
 
if (retry++ = 1000)
return 1;
@@ -1253,8 +1255,7 @@ megasas_build_dcdb(struct megasas_instance *instance, 
struct scsi_cmnd *scp,
  pthru-sgl);
 
if (pthru-sge_count  instance-max_num_sge) {
-   printk(KERN_ERR megasas: DCDB two many SGE NUM=%x\n,
-   pthru-sge_count);
+   pr_err(DCDB two many SGE NUM=%x\n, pthru-sge_count);
return 0;
}
 
@@ -1394,8 +1395,7 @@ megasas_build_ldio(struct megasas_instance *instance, 
struct scsi_cmnd *scp,
ldio-sge_count = megasas_make_sgl32(instance, scp, ldio-sgl);
 
if (ldio-sge_count  instance-max_num_sge) {
-   printk(KERN_ERR megasas: build_ld_io: sge_count = %x\n,
-   ldio-sge_count);
+   pr_err(build_ld_io: sge_count = %x\n, ldio-sge_count);
return 0;
}
 
@@ -1461,63

Re: [PATCH 3/3] [SCSI] mpt2sas: Remove unnecessary use of a boolean variable

[Joe Perches]

On Fri, 2014-12-19 at 12:13 +0100, Quentin Lambert wrote:
[]
 diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c 
 b/drivers/scsi/mpt2sas/mpt2sas_base.c
[]
 @@ -1414,12 +1414,8 @@ _base_enable_msix(struct MPT2SAS_ADAPTER *ioc)
   struct msix_entry *entries, *a;
   int r;
   int i;
 - u8 try_msix = 0;
  
 - if (msix_disable == -1 || msix_disable == 0)
 - try_msix = 1;
 -
 - if (!try_msix)
 + if (msix_disable != -1 || msix_disable != 0)
   goto try_ioapic;

logic not the same.

Now needs  not ||


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] [SCSI] mpt2sas: Convert non-returned local variable to boolean when relevant

[Joe Perches]

On Wed, 2014-12-17 at 15:56 +0100, Quentin Lambert wrote:
 This patch was produced using Coccinelle. A simplified version of the
 semantic patch is:

[...]

Interesting.

Some of these conversions show what I think is
relatively poor logic.  For instance:

 diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c 
 b/drivers/scsi/mpt2sas/mpt2sas_base.c
[]
 @@ -1414,10 +1414,10 @@ _base_enable_msix(struct MPT2SAS_ADAPTER *ioc)
   struct msix_entry *entries, *a;
   int r;
   int i;
 - u8 try_msix = 0;
 + bool try_msix = false;
  
   if (msix_disable == -1 || msix_disable == 0)
 - try_msix = 1;
 + try_msix = true;
  
   if (!try_msix)
   goto try_ioapic;

This might as well remove the try_msix variable
and become:

if (msix_disable != -1  msix_disable != 0)
goto try_ioapic;

 @@ -3236,7 +3236,7 @@ mpt2sas_base_sas_iounit_control(struct MPT2SAS_ADAPTER 
 *ioc,
   u16 smid;
   u32 ioc_state;
   unsigned long timeleft;
 - u8 issue_reset;
 + bool issue_reset;
   int rc;
   void *request;
   u16 wait_state_count;
 @@ -3300,7 +3300,7 @@ mpt2sas_base_sas_iounit_control(struct MPT2SAS_ADAPTER 
 *ioc,
   _debug_dump_mf(mpi_request,
   sizeof(Mpi2SasIoUnitControlRequest_t)/4);
   if (!(ioc-base_cmds.status  MPT2_CMD_RESET))
 - issue_reset = 1;
 + issue_reset = true;
   goto issue_host_reset;
   }
   if (ioc-base_cmds.status  MPT2_CMD_REPLY_VALID)

It seems like issue_reset should be initialized to false.
issue_reset can be an arbitrary value before the goto issue_host_reset
which is:

 issue_host_reset:
if (issue_reset)
mpt2sas_base_hard_reset_handler(ioc, CAN_SLEEP,
FORCE_BIG_HAMMER);

 @@ -3341,7 +3341,7 @@ mpt2sas_base_scsi_enclosure_processor(struct 
 MPT2SAS_ADAPTER *ioc,
   u16 smid;
   u32 ioc_state;p
   unsigned long timeleft;
 - u8 issue_reset;
 + bool issue_reset;
   int rc;
   void *request;
   u16 wait_state_count;
 @@ -3398,7 +3398,7 @@ mpt2sas_base_scsi_enclosure_processor(struct 
 MPT2SAS_ADAPTER *ioc,
   _debug_dump_mf(mpi_request,
   sizeof(Mpi2SepRequest_t)/4);
   if (!(ioc-base_cmds.status  MPT2_CMD_RESET))
 - issue_reset = 1;
 + issue_reset = true;
   goto issue_host_reset;
   }

same

I stopped looking here.


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/20] fix misspelling of current function in string

[Joe Perches]

On Sun, 2014-12-07 at 20:20 +0100, Julia Lawall wrote:
 These patches replace what appears to be a reference to the name of the
 current function but is misspelled in some way by either the name of the
 function itself, or by %s and then __func__ in an argument list.

At least a few of these seem to be function tracing
style uses that might as well be deleted instead.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ips: Remove unnecessary METHOD_TRACE

[Joe Perches]

METHOD_TRACE is a poorman's function tracer.
Use the actual function tracer instead (ftrace)

Signed-off-by: Joe Perches j...@perches.com
---
 drivers/scsi/ips.c | 177 -
 1 file changed, 177 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index e5afc38..a483244 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -153,8 +153,6 @@
  *NOTE: only works when IPS_DEBUG compile directive is 
used.
  *   1  - Normal debug messages
  *   2  - Verbose debug messages
- *   11 - Method trace (non interrupt)
- *   12 - Method trace (includes interrupt)
  *
  * noi2o- Don't use I2O Queues (ServeRAID 4 only)
  * nommap   - Don't use memory mapped I/O
@@ -216,11 +214,9 @@ module_param(ips, charp, 0);
  scb-scsi_cmd-sc_data_direction)
 
 #ifdef IPS_DEBUG
-#define METHOD_TRACE(s, i)if (ips_debug = (i+10)) printk(KERN_NOTICE s 
\n);
 #define DEBUG(i, s)   if (ips_debug = i) printk(KERN_NOTICE s \n);
 #define DEBUG_VAR(i, s, v...) if (ips_debug = i) printk(KERN_NOTICE s \n, 
v);
 #else
-#define METHOD_TRACE(s, i)
 #define DEBUG(i, s)
 #define DEBUG_VAR(i, s, v...)
 #endif
@@ -563,8 +559,6 @@ ips_detect(struct scsi_host_template * SHT)
 {
int i;
 
-   METHOD_TRACE(ips_detect, 1);
-
 #ifdef MODULE
if (ips)
ips_setup(ips);
@@ -654,8 +648,6 @@ ips_release(struct Scsi_Host *sh)
ips_ha_t *ha;
int i;
 
-   METHOD_TRACE(ips_release, 1);
-
scsi_remove_host(sh);
 
for (i = 0; i  IPS_MAX_ADAPTERS  ips_sh[i] != sh; i++) ;
@@ -788,8 +780,6 @@ int ips_eh_abort(struct scsi_cmnd *SC)
int ret;
struct Scsi_Host *host;
 
-   METHOD_TRACE(ips_eh_abort, 1);
-
if (!SC)
return (FAILED);
 
@@ -846,8 +836,6 @@ static int __ips_eh_reset(struct scsi_cmnd *SC)
ips_scb_t *scb;
ips_copp_wait_item_t *item;
 
-   METHOD_TRACE(ips_eh_reset, 1);
-
 #ifdef NO_IPS_RESET
return (FAILED);
 #else
@@ -1052,8 +1040,6 @@ static int ips_queue_lck(struct scsi_cmnd *SC, void 
(*done) (struct scsi_cmnd *)
ips_ha_t *ha;
ips_passthru_t *pt;
 
-   METHOD_TRACE(ips_queue, 1);
-
ha = (ips_ha_t *) SC-device-host-hostdata;
 
if (!ha)
@@ -1157,8 +1143,6 @@ static int ips_biosparam(struct scsi_device *sdev, struct 
block_device *bdev,
int sectors;
int cylinders;
 
-   METHOD_TRACE(ips_biosparam, 1);
-
if (!ha)
/* ?!?! host adater info invalid */
return (0);
@@ -1234,8 +1218,6 @@ do_ipsintr(int irq, void *dev_id)
struct Scsi_Host *host;
int irqstatus;
 
-   METHOD_TRACE(do_ipsintr, 2);
-
ha = (ips_ha_t *) dev_id;
if (!ha)
return IRQ_NONE;
@@ -1281,8 +1263,6 @@ ips_intr_copperhead(ips_ha_t * ha)
IPS_STATUS cstatus;
int intrstatus;
 
-   METHOD_TRACE(ips_intr, 2);
-
if (!ha)
return 0;
 
@@ -1345,8 +1325,6 @@ ips_intr_morpheus(ips_ha_t * ha)
IPS_STATUS cstatus;
int intrstatus;
 
-   METHOD_TRACE(ips_intr_morpheus, 2);
-
if (!ha)
return 0;
 
@@ -1412,8 +1390,6 @@ ips_info(struct Scsi_Host *SH)
char *bp;
ips_ha_t *ha;
 
-   METHOD_TRACE(ips_info, 1);
-
ha = IPS_HA(SH);
 
if (!ha)
@@ -1495,8 +1471,6 @@ static int ips_is_passthru(struct scsi_cmnd *SC)
 {
unsigned long flags;
 
-   METHOD_TRACE(ips_is_passthru, 1);
-
if (!SC)
return (0);
 
@@ -1572,8 +1546,6 @@ ips_make_passthru(ips_ha_t *ha, struct scsi_cmnd *SC, 
ips_scb_t *scb, int intr)
int i, ret;
 struct scatterlist *sg = scsi_sglist(SC);
 
-   METHOD_TRACE(ips_make_passthru, 1);
-
 scsi_for_each_sg(SC, sg, scsi_sg_count(SC), i)
length += sg-length;
 
@@ -1911,8 +1883,6 @@ ips_usrcmd(ips_ha_t * ha, ips_passthru_t * pt, ips_scb_t 
* scb)
IPS_SG_LIST sg_list;
uint32_t cmd_busaddr;
 
-   METHOD_TRACE(ips_usrcmd, 1);
-
if ((!scb) || (!pt) || (!ha))
return (0);
 
@@ -1998,8 +1968,6 @@ ips_cleanup_passthru(ips_ha_t * ha, ips_scb_t * scb)
 {
ips_passthru_t *pt;
 
-   METHOD_TRACE(ips_cleanup_passthru, 1);
-
if ((!scb) || (!scb-scsi_cmd) || (!scsi_sglist(scb-scsi_cmd))) {
DEBUG_VAR(1, (%s%d) couldn't cleanup after passthru,
  ips_name, ha-host_num);
@@ -2036,8 +2004,6 @@ ips_cleanup_passthru(ips_ha_t * ha, ips_scb_t * scb)
 static int
 ips_host_info(ips_ha_t *ha, struct seq_file *m)
 {
-   METHOD_TRACE(ips_host_info, 1);
-
seq_printf(m, \nIBM ServeRAID General Information:\n\n);
 
if ((le32_to_cpu(ha-nvram-signature) == IPS_NVRAM_P5_SIG) 
@@ -2155,8 +2121,6 @@ ips_host_info(ips_ha_t *ha, struct seq_file