RE: [PATCH v2] drivers: visorbus: move driver out of staging

[Kershner, David A]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Friday, December 8, 2017 1:32 PM
> To: Greg KH <gre...@linuxfoundation.org>
> Cc: Kershner, David A <david.kersh...@unisys.com>;
> wadgaonkar...@gmail.com; driverdev-devel@linuxdriverproject.org;
> jes.soren...@gmail.com; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; linux-ker...@vger.kernel.org;
> erik.arfvid...@gmail.com
> Subject: Re: [PATCH v2] drivers: visorbus: move driver out of staging
> 
> On Fri, Dec 08, 2017 at 04:39:13PM +0100, Greg KH wrote:
> > On Thu, Dec 07, 2017 at 12:11:07PM -0500, David Kershner wrote:
> > > Move the visorbus driver out of staging
(drivers/staging/unisys/visorbus)
> > > and to drivers/visorbus. Modify the configuration and makefiles so
they
> > > now reference the new location. The s-Par header file visorbus.h that
is
> > > referenced by all s-Par drivers, is being moved into include/linux.
> > >
> > > Signed-off-by: David Kershner <david.kersh...@unisys.com>
> > > Reviewed-by: Tim Sell <timothy.s...@unisys.com>
> >
> > Nice work, now applied to my testing tree!
> 
> Congrats.  :)
> 

Thanks, and thanks for all the help getting to this point! 

David Kershner

> regards,
> dan carpenter



smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v2] drivers: visorbus: move driver out of staging

[Kershner, David A]

> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Friday, December 8, 2017 11:53 AM
> To: Greg KH <gre...@linuxfoundation.org>; Kershner, David A
> <david.kersh...@unisys.com>
> Cc: jes.soren...@gmail.com; linux-ker...@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; erik.arfvid...@gmail.com;
> wadgaonkar...@gmail.com
> Subject: Re: [PATCH v2] drivers: visorbus: move driver out of staging
> 
> On Fri, 2017-12-08 at 16:39 +0100, Greg KH wrote:
> > On Thu, Dec 07, 2017 at 12:11:07PM -0500, David Kershner wrote:
> > > Move the visorbus driver out of staging
(drivers/staging/unisys/visorbus)
> > > and to drivers/visorbus. Modify the configuration and makefiles so
they
> > > now reference the new location. The s-Par header file visorbus.h that
is
> > > referenced by all s-Par drivers, is being moved into include/linux.
> 
> Perhaps moving visorbus.h and visorbus_private.h to
> drivers/visorbus would be better than adding more
> relatively localized #include files to include/linux/
> 
> what about controlvmchannel?
> 

The file visorbus.h is currently included by 4 drivers: visorbus and 3
other drivers that are staying in staging for now, but we plan to move out
soon.

The files visorbus_private.h and controlvmchannel.h are only used by the
visorbus driver and belong solely in the visorbus directory, no one should
be including them besides the visorbus driver. 

David Kershner


smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/2] drivers: visorbus: move driver out of staging

[Kershner, David A]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Saturday, November 18, 2017 5:26 AM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: jes.soren...@gmail.com; linux-ker...@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; erik.arfvid...@gmail.com;
> wadgaonkar...@gmail.com
> Subject: Re: [PATCH 2/2] drivers: visorbus: move driver out of staging
> 
> On Fri, Nov 17, 2017 at 12:27:39PM -0500, David Kershner wrote:
> >  {drivers/staging/unisys/include => include/linux/visorbus}/visorbus.h |
0
> >  .../staging/unisys/include => include/linux/visorbus}/visorchannel.h  |
0
> 
> Do we really need two different include/linux .h files for this bus
> subsystem?  Can we merge them both together?
>

I will merge them together, they were separate so the other channel
definitions
didn't have to include all of visorbus.h for clarity.
 
> > diff --git a/drivers/staging/unisys/visorbus/Kconfig
> b/drivers/visorbus/Kconfig
> > similarity index 90%
> > rename from drivers/staging/unisys/visorbus/Kconfig
> > rename to drivers/visorbus/Kconfig
> > index 5113880..a99285a 100644
> > --- a/drivers/staging/unisys/visorbus/Kconfig
> > +++ b/drivers/visorbus/Kconfig
> > @@ -4,7 +4,9 @@
> >
> >  config UNISYS_VISORBUS
> > tristate "Unisys visorbus driver"
> > -   depends on UNISYSSPAR
> > +   depends on X86_64 && !UML
> > +   select PCI
> > +   select ACPI
> 
> Wait, what?  Why are you messing with the dependancies now?  And never
> do a select if at all possible, do a 'depends on' instead.
> 
> And why not UML?  What about other arches, why are you now restricting
> it?  That's an odd change to be doing here in a "move this out of
> staging" patch...
> 

When I moved them out of staging, I noticed that it had "depends on
UNISYSSPAR"
flag. The dependencies I added were copied from the UNISYSSPAR dependency
list. 

I will make a separate patch to switch them over to depends and determine if
the
!UML flag is still needed.

Thanks, 
David Kershner

> thanks,
> 
> greg k-h


smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/2] drivers: visorbus: move driver out of staging

[Kershner, David A]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Friday, November 17, 2017 2:18 PM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: gre...@linuxfoundation.org; jes.soren...@gmail.com; linux-
> ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer <sparmaintai...@unisys.com>; erik.arfvid...@gmail.com;
> wadgaonkar...@gmail.com
> Subject: Re: [PATCH 2/2] drivers: visorbus: move driver out of staging
> 
> Please don' tcreate new subdirectories under include/linux
> if you don't have to.
> 

Thanks for the feedback, the s-Par drivers have 3 include files in the
 include directory in drivers/staging/unisys/include. The patch currently
 moves 2 of them, and the third will be moved when the other drivers
 get out of staging. When I did the move, I thought one directory with
 three files would be cleaner than just adding three files to include. I
 will change that.

> Also who outside of unisys has reviewed this whole code?
> 

The driver has been in staging for 4 years with significant rework during
that period of time. Throughout that time, we have had input from several
different engineers, including Dan Carpenter, Jes Sorenson, and Greg KH.
In October, I requested a formal review from the community and after the
review had completed, Greg gave us the okay to move them out of staging.

> Instead of a move please send an actual patchset to add the new files
> so people can review it just like any other code.

Okay, I'll redo the patchset to show the explicit add of the files to
 the drivers directory.

Thanks,
David Kershner



smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 10/11] staging: most: add SPDX identifiers to all unisys driver files

[Kershner, David A]

> -Original Message-
> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On
> Behalf Of Greg Kroah-Hartman
> Sent: Tuesday, November 7, 2017 8:59 AM
> To: de...@driverdev.osuosl.org
> Cc: Kate Stewart ; Greg Kroah-Hartman
> ; Michael Fabry
> ; Philippe Ombredanne
> ; Christian Gromm
> ; Thomas Gleixner 
> Subject: [PATCH 10/11] staging: most: add SPDX identifiers to all unisys
driver
> files
> 

The headline says "unisys driver files", but the patch is for the most
driver.

David Kershner

> It's good to have SPDX identifiers in all files to make it easier to
> audit the kernel tree for correct licenses.
> 
> Update the drivers/staging/most files files with the correct SPDX
> license identifier based on the license text in the file itself.  The
> SPDX identifier is a legally binding shorthand, which can be used
> instead of the full boiler plate text.
> 
> This work is based on a script and data from Thomas Gleixner, Philippe
> Ombredanne, and Kate Stewart.
> 
> Cc: Michael Fabry 
> Cc: Christian Gromm 
> Cc: Thomas Gleixner 
> Cc: Kate Stewart 
> Cc: Philippe Ombredanne 
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/staging/most/aim-cdev/cdev.c  | 1 +
>  drivers/staging/most/aim-network/networking.c | 1 +
>  drivers/staging/most/aim-sound/sound.c| 1 +
>  drivers/staging/most/aim-v4l2/video.c | 1 +
>  drivers/staging/most/hdm-dim2/dim2_errors.h   | 1 +
>  drivers/staging/most/hdm-dim2/dim2_hal.c  | 1 +
>  drivers/staging/most/hdm-dim2/dim2_hal.h  | 1 +
>  drivers/staging/most/hdm-dim2/dim2_hdm.c  | 1 +
>  drivers/staging/most/hdm-dim2/dim2_hdm.h  | 1 +
>  drivers/staging/most/hdm-dim2/dim2_reg.h  | 1 +
>  drivers/staging/most/hdm-dim2/dim2_sysfs.c| 1 +
>  drivers/staging/most/hdm-dim2/dim2_sysfs.h| 1 +
>  drivers/staging/most/hdm-i2c/hdm_i2c.c| 1 +
>  drivers/staging/most/hdm-usb/hdm_usb.c| 1 +
>  drivers/staging/most/mostcore/core.c  | 1 +
>  drivers/staging/most/mostcore/mostcore.h  | 1 +
>  16 files changed, 16 insertions(+)
> 
> diff --git a/drivers/staging/most/aim-cdev/cdev.c
> b/drivers/staging/most/aim-cdev/cdev.c
> index 1e5cbc893496..d6e7e85d9ece 100644
> --- a/drivers/staging/most/aim-cdev/cdev.c
> +++ b/drivers/staging/most/aim-cdev/cdev.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * cdev.c - Application interfacing module for character devices
>   *
> diff --git a/drivers/staging/most/aim-network/networking.c
> b/drivers/staging/most/aim-network/networking.c
> index 936f013c350e..e2dff6d5cf30 100644
> --- a/drivers/staging/most/aim-network/networking.c
> +++ b/drivers/staging/most/aim-network/networking.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Networking AIM - Networking Application Interface Module for MostCore
>   *
> diff --git a/drivers/staging/most/aim-sound/sound.c
> b/drivers/staging/most/aim-sound/sound.c
> index ea1366a44008..0f596ca532d8 100644
> --- a/drivers/staging/most/aim-sound/sound.c
> +++ b/drivers/staging/most/aim-sound/sound.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * sound.c - Audio Application Interface Module for Mostcore
>   *
> diff --git a/drivers/staging/most/aim-v4l2/video.c
> b/drivers/staging/most/aim-v4l2/video.c
> index e0748416aee5..e089d139b943 100644
> --- a/drivers/staging/most/aim-v4l2/video.c
> +++ b/drivers/staging/most/aim-v4l2/video.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * V4L2 AIM - V4L2 Application Interface Module for MostCore
>   *
> diff --git a/drivers/staging/most/hdm-dim2/dim2_errors.h
> b/drivers/staging/most/hdm-dim2/dim2_errors.h
> index 66343ba426c1..073d93cf4927 100644
> --- a/drivers/staging/most/hdm-dim2/dim2_errors.h
> +++ b/drivers/staging/most/hdm-dim2/dim2_errors.h
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * dim2_errors.h - Definitions of errors for DIM2 HAL API
>   * (MediaLB, Device Interface Macro IP, OS62420)
> diff --git a/drivers/staging/most/hdm-dim2/dim2_hal.c
> b/drivers/staging/most/hdm-dim2/dim2_hal.c
> index 91484643d289..f0a45863c8cb 100644
> --- a/drivers/staging/most/hdm-dim2/dim2_hal.c
> +++ b/drivers/staging/most/hdm-dim2/dim2_hal.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * dim2_hal.c - DIM2 HAL implementation
>   * (MediaLB, Device Interface Macro IP, OS62420)
> diff --git a/drivers/staging/most/hdm-dim2/dim2_hal.h
> b/drivers/staging/most/hdm-dim2/dim2_hal.h
> index 6df6ea5f7da4..df678124117c 100644
> --- a/drivers/staging/most/hdm-dim2/dim2_hal.h
> +++ b/drivers/staging/most/hdm-dim2/dim2_hal.h
> @@ -1,3 +1,4 @@
> +// 

RE: [PATCH] staging: unisys: visorchipset: Use common error handling code in setup_crash_devices_work_queue()

[Kershner, David A]

> -Original Message-
> From: SF Markus Elfring [mailto:elfr...@users.sourceforge.net]
> Sent: Friday, November 3, 2017 3:50 PM
> To: de...@driverdev.osuosl.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; Thompson, Bryan E.
> <bryan.thomp...@unisys.com>; Binder, David Anthony
> <david.bin...@unisys.com>; Kershner, David A
> <david.kersh...@unisys.com>; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; Sameer Wadgaonkar
> <sameer.wadgaon...@unisys.com>; Sell, Timothy C
> <timothy.s...@unisys.com>
> Cc: LKML <linux-ker...@vger.kernel.org>; kernel-janit...@vger.kernel.org
> Subject: [PATCH] staging: unisys: visorchipset: Use common error handling
> code in setup_crash_devices_work_queue()
>
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Fri, 3 Nov 2017 20:37:03 +0100
>
> * Add a jump target so that a specific error message is stored only once
>   at the end of this function implementation.
>
> * Replace four calls of the function "dev_err" by goto statements.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Thanks for the catch!


Acked-by: David Kershner <david.kersh...@unisys.com>



> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 36 
> 
> --
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index fed554a43151..1d54821dd7b6 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -1231,11 +1231,9 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> offsetof(struct visor_controlvm_channel,
>  saved_crash_message_count),
> -   _crash_msg_count, sizeof(u16)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   _crash_msg_count, sizeof(u16)) < 0)
> + goto report_read_failure;
> +
>   if (local_crash_msg_count != CONTROLVM_CRASHMSG_MAX) {
>   dev_err(_dev->acpi_device->dev, "invalid
> count\n");
>   return;
> @@ -1244,30 +1242,24 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> offsetof(struct visor_controlvm_channel,
>  saved_crash_message_offset),
> -   _crash_msg_offset, sizeof(u32)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   _crash_msg_offset, sizeof(u32)) < 0)
> + goto report_read_failure;
> +
>   /* read create device message for storage bus offset */
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> local_crash_msg_offset,
> _crash_bus_msg,
> -   sizeof(struct controlvm_message)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   sizeof(struct controlvm_message)) < 0)
> + goto report_read_failure;
> +
>   /* read create device message for storage device */
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> local_crash_msg_offset +
> sizeof(struct controlvm_message),
> _crash_dev_msg,
> -   sizeof(struct controlvm_message)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   sizeof(struct controlvm_message)) < 0)
> + goto report_read_failure;
> +
>   /* reuse IOVM create bus message */
>   if (!local_crash_bus_msg.cmd.create_bus.channel_addr) {
>   dev_err(_dev->acpi_device->dev,
> @@ -1282,6 +1274,10 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   return;
>   }
>   visorbus_device_create(_crash_dev_msg);
> + return;
> +
> +report_read_failure:
> + dev_err(_dev->acpi_device->dev, "failed to read
> channel\n");
>  }
>
>  void visorbus_response(struct visor_device *bus_info, int response,
> --
> 2.15.0



smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v2 1/1] staging: Fix incorrect unisys MAINTAINERS pattern

[Kershner, David A]

> -Original Message-
> From: Tom Saeger [mailto:tom.sae...@oracle.com]
> Sent: Tuesday, October 31, 2017 11:30 AM
> To: Kershner, David A <david.kersh...@unisys.com>; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Cc: Tom Saeger <tom.sae...@oracle.com>
> Subject: [PATCH v2 1/1] staging: Fix incorrect unisys MAINTAINERS pattern
> 
> Fix stale path to documentation in MAINTAINERS file.
> 
> Signed-off-by: Tom Saeger <tom.sae...@oracle.com>
> Cc: David Kershner <david.kersh...@unisys.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>

Acked-by: David Kershner <david.kers...@unisys.com>

> ---
>  drivers/staging/unisys/MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/MAINTAINERS
> b/drivers/staging/unisys/MAINTAINERS
> index 1f0425bf3583..aaddc619c329 100644
> --- a/drivers/staging/unisys/MAINTAINERS
> +++ b/drivers/staging/unisys/MAINTAINERS
> @@ -1,5 +1,5 @@
>  Unisys s-Par drivers
>  M:   David Kershner <sparmaintai...@unisys.com>
>  S:   Maintained
> -F:   Documentation/s-Par/overview.txt
> +F:   drivers/staging/unisys/Documentation/overview.txt
>  F:   drivers/staging/unisys/
> --
> 2.14.3



smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys/visorbus: add __init/__exit annotations

[Kershner, David A]

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Friday, September 15, 2017 3:23 PM
> To: Kershner, David A <david.kersh...@unisys.com>; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>
> Cc: Arnd Bergmann <a...@arndb.de>; Sell, Timothy C
> <timothy.s...@unisys.com>; Wadgaonkar, Sameer Laxmikant
> <sameer.wadgaon...@unisys.com>; Binder, David Anthony
> <david.bin...@unisys.com>; Thompson, Bryan E.
> <bryan.thomp...@unisys.com>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] staging: unisys/visorbus: add __init/__exit annotations
> 
> gcc-4.6 causes a harmless warning about the init function:
> 
> WARNING: vmlinux.o(.text+0xed62c2): Section mismatch in reference from
> the function init_unisys() to the function
.init.text:visorutil_spar_detect()
> The function init_unisys() references
> the function __init visorutil_spar_detect().
> This is often because init_unisys lacks a __init
> annotation or the annotation of visorutil_spar_detect is wrong.
> 
> It appears that newer versions inline visorutil_spar_detect(),
> end up with an empty __init section. This marks the module
> entry points as __init and __exit respectively, which avoids
> the warning and slightly reduces the runtime code size.
> 
> Signed-off-by: Arnd Bergmann <a...@arndb.de>

Acked-by: David Kershner <david.kersh...@unisys.com>

Thanks,
David Kershner

> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index 74cce4f1a7bd..27ecf6fb49fd 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -1826,7 +1826,7 @@ static __init int visorutil_spar_detect(void)
>   return 0;
>  }
> 
> -static int init_unisys(void)
> +static int __init init_unisys(void)
>  {
>   int result;
> 
> @@ -1841,7 +1841,7 @@ static int init_unisys(void)
>   return 0;
>  };
> 
> -static void exit_unisys(void)
> +static void __exit exit_unisys(void)
>  {
>   acpi_bus_unregister_driver(_acpi_driver);
>  }
> --
> 2.9.0



smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH][staging-next] staging: unisys: visorbus: make two functions static

[Kershner, David A]

> -Original Message-
> From: Colin King [mailto:colin.k...@canonical.com]
> Sent: Friday, September 1, 2017 6:08 AM
> To: Greg Kroah-Hartman ; Sell, Timothy C
> ; Binder, David Anthony
> ; Wadgaonkar, Sameer Laxmikant
> ; Charles Daniels
> ; Andy Shevchenko
> ; *S-Par-Maintainer
> ; de...@driverdev.osuosl.org
> Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: [PATCH][staging-next] staging: unisys: visorbus: make two functions
> static
>
> From: Colin Ian King 
>
> The functions sig_queue_offset and sig_data_offset are local to
> the source and do not need to be in global scope, so make them
> static.
>
> Cleans up sparse warnings:
> symbol 'sig_queue_offset' was not declared. Should it be static?
> symbol 'sig_data_offset' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King 

Acked-by: David Kershner 

> ---
>  drivers/staging/unisys/visorbus/visorchannel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>



smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v1] staging: unisys: visorinput: Add module_driver driver registration

[Kershner, David A]

> -Original Message-
> From: Alex Briskin [mailto:br.shu...@gmail.com]
> Sent: Friday, August 25, 2017 12:25 PM
> To: *S-Par-Maintainer 
> Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org
> Subject: [PATCH v1] staging: unisys: visorinput: Add module_driver driver
> registration
> 
> 1. Remove module_init()/module_exit() macroes and
> visorbus_register_visor_driver/visorbus_unregister_visor_driver
> functions.
> 2. Replace with a short module_driver macro
> 
> Signed-off-by: Alex Briskin 

Tested on s-Par. 
Acked-by: David Kershner 

Thanks, 
David Kershner


smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 00/45] staging: unisys: more changes to the code.

[Kershner, David A]

> -Original Message-
> From: David Kershner [mailto:david.kersh...@unisys.com]
> Sent: Tuesday, August 1, 2017 10:40 AM
> To: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> *S-Par-Maintainer <sparmaintai...@unisys.com>; jes.soren...@gmail.com
> Cc: Kershner, David A <david.kersh...@unisys.com>
> Subject: [PATCH 00/45] staging: unisys: more changes to the code.
> 
> The following patch series addresses issues from internal code-review as
> well as checkpatch errors and suggestions from GregKH.
> 
> We have updated our commenting style to be more readable (and adopted a
> kernel-doc like format). We have intentionally not made them kernel-doc by
> removing the extra asterisk from the first line.
> 

Greg, I just realized that although the patchset applies cleanly to
staging-next, it will conflict with the patch:

Subject: [PATCH v2] staging: unisys: Switch to use new generic UUID
API
From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com]

So feel free to drop the series and accept his and I can fix the conflicts,
or
do you want to drop all the patches and I pick it up and resend the series
with the patch included?

Thanks, 
David Kershner


smime.p7s
Description: S/MIME cryptographic signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v2] staging: unisys: Switch to use new generic UUID API

[Kershner, David A]

> -Original Message-
> From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com]
> Sent: Monday, July 31, 2017 1:19 PM
> Subject: [PATCH v2] staging: unisys: Switch to use new generic UUID API
> 
> There are new types and helpers that are supposed to be used in new code.
> 
> As a preparation to get rid of legacy types and API functions do
> the conversion here.
> 
> While here, re-indent couple of lines to increase readability.
> 
> Cc: David Kershner 
> Cc: Greg Kroah-Hartman 
> Cc: sparmaintai...@unisys.com
> Cc: de...@driverdev.osuosl.org
> Signed-off-by: Andy Shevchenko 

Acked-by: David Kershner 

> ---
>  drivers/staging/unisys/Documentation/overview.txt  | 14 +++
>  drivers/staging/unisys/include/channel.h   | 29 ++
>  drivers/staging/unisys/include/iochannel.h |  2 +-
>  drivers/staging/unisys/include/visorbus.h  | 14 +++
>  drivers/staging/unisys/visorbus/controlvmchannel.h | 16 
>  drivers/staging/unisys/visorbus/vbuschannel.h  |  8 ++--
>  drivers/staging/unisys/visorbus/visorbus_main.c| 44 ++--
> -
>  drivers/staging/unisys/visorbus/visorbus_private.h |  6 +--
>  drivers/staging/unisys/visorbus/visorchannel.c | 46 +++--
> -
>  drivers/staging/unisys/visorbus/visorchipset.c | 33 +---
>  drivers/staging/unisys/visorhba/visorhba_main.c|  6 +--
>  drivers/staging/unisys/visorinput/visorinput.c | 38 +-
>  drivers/staging/unisys/visornic/visornic_main.c|  6 +--
>  13 files changed, 130 insertions(+), 132 deletions(-)

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] Staging : unisys : visorbus : visorbus_main: Fixed a brace cdoing style issue

[Kershner, David A]

> -Original Message-
> From: Himanshu Jha [mailto:himanshujha199...@gmail.com]
> Subject: [PATCH] Staging : unisys : visorbus : visorbus_main: Fixed a brace
> cdoing style issue
> 
> Fixed coding style issue for function declaration.
> 

This doesn't apply to Greg's branch, fix was already applied. Patch that
fixed it was:

7b9de71b staging: unisys: visorbus: fix improper bracket blocks

Thanks, 
David Kershner

> Signed-off-by: Himanshu Jha 
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 1c785dd..c564962 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -270,7 +270,8 @@ static const struct attribute_group
> *visorbus_channel_groups[] = {
> 
>  static ssize_t partition_handle_show(struct device *dev,
>struct device_attribute *attr,
> -  char *buf) {
> +  char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   u64 handle = visorchannel_get_clientpartition(vdev->visorchannel);
> 
> @@ -280,7 +281,8 @@ static DEVICE_ATTR_RO(partition_handle);
> 
>  static ssize_t partition_guid_show(struct device *dev,
>  struct device_attribute *attr,
> -char *buf) {
> +char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
> 
>   return sprintf(buf, "{%pUb}\n", >partition_uuid);
> @@ -289,7 +291,8 @@ static DEVICE_ATTR_RO(partition_guid);
> 
>  static ssize_t partition_name_show(struct device *dev,
>  struct device_attribute *attr,
> -char *buf) {
> +char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
> 
>   return sprintf(buf, "%s\n", vdev->name);
> @@ -298,7 +301,8 @@ static DEVICE_ATTR_RO(partition_name);
> 
>  static ssize_t channel_addr_show(struct device *dev,
>struct device_attribute *attr,
> -  char *buf) {
> +  char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   u64 addr = visorchannel_get_physaddr(vdev->visorchannel);
> 
> @@ -308,7 +312,8 @@ static DEVICE_ATTR_RO(channel_addr);
> 
>  static ssize_t channel_bytes_show(struct device *dev,
> struct device_attribute *attr,
> -   char *buf) {
> +   char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   u64 nbytes = visorchannel_get_nbytes(vdev->visorchannel);
> 
> @@ -318,7 +323,8 @@ static DEVICE_ATTR_RO(channel_bytes);
> 
>  static ssize_t channel_id_show(struct device *dev,
>  struct device_attribute *attr,
> -char *buf) {
> +char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   int len = 0;
> 
> --
> 2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: visorbus: constify attribute_group structures.

[Kershner, David A]

> -Original Message-
> From: Greg KH [mailto:g...@kroah.com]
> Sent: Monday, July 17, 2017 8:38 AM
> To: Arvind Yadav <arvind.yadav...@gmail.com>
> Cc: Kershner, David A <david.kersh...@unisys.com>; Sell, Timothy C
> <timothy.s...@unisys.com>; Thompson, Bryan E.
> <bryan.thomp...@unisys.com>; jon.fri...@unisys.com; Binder, David
> Anthony <david.bin...@unisys.com>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] staging: unisys: visorbus: constify attribute_group
> structures.
> 
> On Mon, Jul 17, 2017 at 05:43:14PM +0530, Arvind Yadav wrote:
> > Hi Greg,
> >
> >
> > On Monday 17 July 2017 04:15 PM, Greg KH wrote:
> > > On Mon, Jul 17, 2017 at 02:55:37PM +0530, Arvind Yadav wrote:
> > > > attribute_groups are not supposed to change at runtime. All functions
> > > > working with attribute_groups provided by  work
> > > > with const attribute_group. So mark the non-const structs as const.
> > > >
> > > > Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
> > > > ---
> > > >   drivers/staging/unisys/visorbus/visorbus_main.c | 4 ++--
> > > >   drivers/staging/unisys/visorbus/visorchipset.c  | 2 +-
> > > >   2 files changed, 3 insertions(+), 3 deletions(-)
> > > Why not just use the ATTRIBUTE_GROUPS() macro for these?  Or is there
> > > something that is preventing that?
> > Yes, we can use. if we are only initializing '.attrs'.
> > ATTRIBUTE_GROUPS() will not work if we will initialize other member of
> > attribute_group like 'bin_attrs', 'is_visible', and  'name'.
> 
> That means you should redo this patch :)
> 
> Also, your changelog text had a typo, it is "attribute_group", not
> "attribute_groups".
> 

Greg, are you recommending that we shouldn't be setting the attribute_group
.name field? What does it pick up if we don't specify it?

Also, for our attribute_groups in visorchipset, we are defining it with two
different attribute_group variables. Are you allowed to use two different
attribute_group variables in an attribute_groups, or is this frowned upon and
we should flatten it out to just one? An example that we used in the kernel was:

static const struct attribute_group *l2_cache_pmu_attr_grps[] = {
_cache_pmu_format_group,
_cache_pmu_cpumask_group,
NULL,
};

Thanks,
David Kershner

> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH]

[Kershner, David A]

> -Original Message-
> From: armetallica [mailto:armetall...@gmail.com]
> Sent: Sunday, July 9, 2017 7:58 PM
> To: gre...@linuxfoundation.org
> Cc: *S-Par-Maintainer ;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> Subject: [PATCH]
> 
> From bb1aac6ae6b21b903d8743712e21aeb1a6b22163 Mon Sep 17 00:00:00
> 2001
> From: Armin Schoenlieb 
> Date: Mon, 10 Jul 2017 01:52:41 +0200
> Subject: [PATCH] staging: unisys: visorbus: fix brace coding style issue in
>  visorbus_main.c This is a patch to the visorbus_main.c file that fixes up six
>  brace errors found by the checkpatch.pl tool
> 

Thanks for the patch! It looks like I sent up a patch to Greg that addresses 
this
issue on 6/30/2017:
 
[PATCH 25/25] staging: unisys: visorbus: fix improper bracket blocks

Also, your patch come in with a subject of just the word [PATCH] and the patch
comment was messed up. I'm not sure how you generated the patch, but you
might want to try it with format-patch and then do a git send-email to yourself
as a dry-run to see if the patch looks OK when you get it.

David Kershner


> Signed-off-by: Armin Schoenlieb 
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 1c785dd19ddd..c56496269fc8 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -270,7 +270,8 @@ static const struct attribute_group
> *visorbus_channel_groups[] = {
> 
>  static ssize_t partition_handle_show(struct device *dev,
>struct device_attribute *attr,
> -  char *buf) {
> +  char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   u64 handle = visorchannel_get_clientpartition(vdev->visorchannel);
> 
> @@ -280,7 +281,8 @@ static DEVICE_ATTR_RO(partition_handle);
> 
>  static ssize_t partition_guid_show(struct device *dev,
>  struct device_attribute *attr,
> -char *buf) {
> +char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
> 
>   return sprintf(buf, "{%pUb}\n", >partition_uuid);
> @@ -289,7 +291,8 @@ static DEVICE_ATTR_RO(partition_guid);
> 
>  static ssize_t partition_name_show(struct device *dev,
>  struct device_attribute *attr,
> -char *buf) {
> +char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
> 
>   return sprintf(buf, "%s\n", vdev->name);
> @@ -298,7 +301,8 @@ static DEVICE_ATTR_RO(partition_name);
> 
>  static ssize_t channel_addr_show(struct device *dev,
>struct device_attribute *attr,
> -  char *buf) {
> +  char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   u64 addr = visorchannel_get_physaddr(vdev->visorchannel);
> 
> @@ -308,7 +312,8 @@ static DEVICE_ATTR_RO(channel_addr);
> 
>  static ssize_t channel_bytes_show(struct device *dev,
> struct device_attribute *attr,
> -   char *buf) {
> +   char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   u64 nbytes = visorchannel_get_nbytes(vdev->visorchannel);
> 
> @@ -318,7 +323,8 @@ static DEVICE_ATTR_RO(channel_bytes);
> 
>  static ssize_t channel_id_show(struct device *dev,
>  struct device_attribute *attr,
> -char *buf) {
> +char *buf)
> +{
>   struct visor_device *vdev = to_visor_device(dev);
>   int len = 0;
> 
> --
> 2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] Replace the literal function name "visorbus_create_instance" with the format specifier "%s" so it can be dynamically filled by the __func__ macro.

[Kershner, David A]

> -Original Message-
> From: Quytelda Kahja [mailto:quyte...@tamalin.org]
> Sent: Tuesday, June 27, 2017 5:18 PM
> To: Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org
> Cc: de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; Quytelda
> Kahja <quyte...@tamalin.org>
> Subject: [PATCH] Replace the literal function name
> "visorbus_create_instance" with the format specifier "%s" so it can be
> dynamically filled by the __func__ macro.

I think the subject line got lost somehow, and should this have been a v2? 

Subject line should start with staging: unisys: visorbus: preferably.

David Kershner

> 
> Signed-off-by: Quytelda Kahja <quyte...@tamalin.org>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 1c785dd19ddd..1c6dc3a3e64a 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -1030,7 +1030,7 @@ visorbus_create_instance(struct visor_device
> *dev)
>  err_debugfs_dir:
>   debugfs_remove_recursive(dev->debugfs_dir);
>   kfree(hdr_info);
> - dev_err(>device, "visorbus_create_instance failed: %d\n",
> err);
> + dev_err(>device, "%s failed: %d\n", __func__, err);
>   return err;
>  }
> 
> --
> 2.13.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] Staging: visorbus: Fix coding style warning from checkpatch.pl.

[Kershner, David A]

> -Original Message-
> From: Quytelda Kahja [mailto:quyte...@tamalin.org]
> Sent: Monday, June 26, 2017 7:32 PM
> To: Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org; Sell, Timothy C <timothy.s...@unisys.com>;
> Binder, David Anthony <david.bin...@unisys.com>
> Cc: de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; Quytelda
> Kahja <quyte...@tamalin.org>
> Subject: [PATCH] Staging: visorbus: Fix coding style warning from
> checkpatch.pl.
> 
> Replace the literal function name "create_bus_instance" with the format
> specifier "%s" so it can be dynamically filled by the __func__ macro.
> 
> Signed-off-by: Quytelda Kahja <quyte...@tamalin.org>

I can't get this to apply to Greg's staging-next branch. It appears to be 
missing
at least patch: 

commit 9e78fd35d staging: unisys: visorbus: renamed functions like 
*_bus_instance 
   to match driver namespace

Please update and send again. 

Thanks, 
David Kershner

> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index a692561c81c8..b6caa7a98692 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -1032,7 +1032,7 @@ create_bus_instance(struct visor_device *dev)
>  err_debugfs_dir:
>   debugfs_remove_recursive(dev->debugfs_dir);
>   kfree(hdr_info);
> - dev_err(>device, "create_bus_instance failed: %d\n", err);
> + dev_err(>device, "%s failed: %d\n", __func__, err);
>   return err;
>  }
> 
> --
> 2.13.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: visorbus: constify visorchipset_parahotplug_group

[Kershner, David A]

> -Original Message-
> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On
> Behalf Of Arvind Yadav
> Sent: Friday, June 23, 2017 2:31 AM
> To: gre...@linuxfoundation.org; Sell, Timothy C
> ; Wadgaonkar, Sameer Laxmikant
> ; jon.fri...@unisys.com
> Cc: de...@driverdev.osuosl.org; *S-Par-Maintainer
> ; linux-ker...@vger.kernel.org
> Subject: [PATCH] staging: unisys: visorbus: constify
> visorchipset_parahotplug_group
> 
> File size before:
>text  data bss dec hex filename
>   11058   816  24   118982e7a
>   drivers/staging/unisys/visorbus/visorchipset.o
> 
> File size After adding 'const':
>text  data bss dec hex filename
>   11122   752  24   118982e7a
>   drivers/staging/unisys/visorbus/visorchipset.o
> 
> Signed-off-by: Arvind Yadav 
Acked-by: David Kershner 

> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index 4cfd0fa..5a499fa 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -1151,7 +1151,7 @@ static ssize_t deviceenabled_store(struct device
> *dev,
>   NULL
>  };
> 
> -static struct attribute_group visorchipset_parahotplug_group = {
> +static const struct attribute_group visorchipset_parahotplug_group = {
>   .name = "parahotplug",
>   .attrs = visorchipset_parahotplug_attrs
>  };
> --
> 1.9.1
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: visorbus: constify channel_attr_grp and dev_attr_grp

[Kershner, David A]

> -Original Message-
> From: Arvind Yadav [mailto:arvind.yadav...@gmail.com]
> Sent: Friday, June 23, 2017 3:37 AM
> To: Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org; Thompson, Bryan E.
> <bryan.thomp...@unisys.com>; Wadgaonkar, Sameer Laxmikant
> <sameer.wadgaon...@unisys.com>; jon.fri...@unisys.com
> Cc: *S-Par-Maintainer <sparmaintai...@unisys.com>;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] staging: unisys: visorbus: constify channel_attr_grp and
> dev_attr_grp
> 
> File size before:
>text  data bss dec hex filename
>6712   960 52181932001
>   drivers/staging/unisys/visorbus/visorbus_main.o
> 
> File size After adding 'const':
>text  data bss dec hex filename
>6840   832 52181932001
>   drivers/staging/unisys/visorbus/visorbus_main.o
> 
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
Acked-by: David Kershner <david.kersh...@unisys.com>

> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index a692561..26f9885 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -249,7 +249,7 @@ static ssize_t typename_show(struct device *dev,
> struct device_attribute *attr,
>   NULL
>  };
> 
> -static struct attribute_group channel_attr_grp = {
> +static const struct attribute_group channel_attr_grp = {
>   .name = "channel",
>   .attrs = channel_attrs,
>  };
> @@ -340,7 +340,7 @@ static ssize_t channel_id_show(struct device *dev,
>   NULL
>  };
> 
> -static struct attribute_group dev_attr_grp = {
> +static const struct attribute_group dev_attr_grp = {
>   .attrs = dev_attrs,
>  };
> 
> --
> 1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH V2] staging: unisys: visorhba - style fix

[Kershner, David A]

> -Original Message-
> From: Derek Robson [mailto:robso...@gmail.com]
> Sent: Friday, June 16, 2017 11:13 PM
> To: Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org; Sell, Timothy C <timothy.s...@unisys.com>;
> Binder, David Anthony <david.bin...@unisys.com>; Wadgaonkar, Sameer
> Laxmikant <sameer.wadgaon...@unisys.com>;
> marcos.souza@gmail.com; robso...@gmail.com
> Cc: *S-Par-Maintainer <sparmaintai...@unisys.com>;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> Subject: [PATCH V2] staging: unisys: visorhba - style fix
> 
> Fixed style of permissions to octal.
> Found using checkpatch
> 
> Signed-off-by: Derek Robson <robso...@gmail.com>

I applied it and tested it. Though when I applied it, the V1 comment was 
included in the patch commit. 


> 
> V1 has vauge subject
> ---
>  drivers/staging/unisys/visorhba/visorhba_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 2fd31c9762c6..a6e7a6bbc428 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -1090,7 +1090,7 @@ static int visorhba_probe(struct visor_device *dev)
>   goto err_scsi_remove_host;
>   }
>   devdata->debugfs_info =
> - debugfs_create_file("info", S_IRUSR | S_IRGRP,
> + debugfs_create_file("info", 0440,
>   devdata->debugfs_dir, devdata,
>   _debugfs_fops);
>   if (!devdata->debugfs_info) {
> --
> 2.13.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] Drivers: unisys: visorhba - style fix

[Kershner, David A]

> -Original Message-
> From: Derek Robson [mailto:robso...@gmail.com]
> Sent: Friday, June 9, 2017 1:16 AM
> To: Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org; Binder, David Anthony
> <david.bin...@unisys.com>; Sell, Timothy C <timothy.s...@unisys.com>;
> Wadgaonkar, Sameer Laxmikant <sameer.wadgaon...@unisys.com>;
> Thompson, Bryan E. <bryan.thomp...@unisys.com>; robso...@gmail.com
> Cc: *S-Par-Maintainer <sparmaintai...@unisys.com>;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] Drivers: unisys: visorhba - style fix
> 
> Fixed style of permissions to octal.
> Found using checkpatch
> 
> Signed-off-by: Derek Robson <robso...@gmail.com>

Acked-by: David Kershner <david.kersh...@unisys.com>

> ---
>  drivers/staging/unisys/visorhba/visorhba_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 2fd31c9762c6..a6e7a6bbc428 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -1090,7 +1090,7 @@ static int visorhba_probe(struct visor_device *dev)
>   goto err_scsi_remove_host;
>   }
>   devdata->debugfs_info =
> - debugfs_create_file("info", S_IRUSR | S_IRGRP,
> + debugfs_create_file("info", 0440,
>   devdata->debugfs_dir, devdata,
>   _debugfs_fops);
>   if (!devdata->debugfs_info) {
> --
> 2.13.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus

[Kershner, David A]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Tuesday, June 6, 2017 11:06 AM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: cor...@lwn.net; t...@linutronix.de; mi...@kernel.org; akpm@linux-
> foundation.org; jes.soren...@gmail.com; linux-ker...@vger.kernel.org;
> linux-...@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer <sparmaintai...@unisys.com>
> Subject: Re: [PATCH 0/3] move visorbus out of staging to
> drivers/virt/visorbus
> 
> On Tue, Jun 06, 2017 at 04:54:30PM +0200, Greg KH wrote:
> > On Tue, Jun 06, 2017 at 04:53:22PM +0200, Greg KH wrote:
> > > On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > > > On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> > > > > This patchset moves drivers/staging/unisys/include to
> > > > > include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> > > > > drivers/virt/visorbus.
> > > >
> > > > Um, are you thinking it is ready to be moved?  Have you asked for
> > > > another review?
> > > >

Thank you for taking a quick look at our patch series. Part of the motivation
behind this submission was, in fact, to initiate another code review. What is
the formal procedure for initiating a code review?

> > > > In a totally random chance, I was doing some driver core work today
> and
> > > > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > > > have 2 tabs for your 'struct attribute' variables, which is really odd.
> > > >

Sorry I missed that; I guess my eyes glazed over by the time I got to that file,
and I was expecting checkpatch to catch that. Now I know better, and I will be
looking for more things. Thanks for catching.

> > > > Also, you should be using the ATTRIBUTE_GROUPS() macro for them
> instead
> > > > of having to "open code" the struct attribute_group lists.
> > > >
> > > > So either you all have horrible luck in that I just happened to find the
> > > > only remaining problem, or that you should proabably ask for a good
> code
> > > > audit, I haven't looked at the code before today since the last round of
> > > > "fun" I found in just one other random file :)
> > >
> > > Also, many of the attribute callbacks in that file seem to all have
> > > their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> > > catch that...
> > >
> > > partition_handle_show() is one such example that is obviously wrong.
> > >
> > > There's also one checkpatch.pl warning for it, which should probably be
> > > resolved as well.
> >
> > drivers/staging/unisys/visorbus/visorbus_main.c:1035: WARNING: Prefer
> using '"%s...", __func__' to using 'create_bus_instance', this function's 
> name,
> in a string
> >
> > to be specific, something you should have caught, right?
> >
> > Are you sure this is ready to be moved out of staging?  :(
> 
> Eek, I can't look away...
> 
> You do this a bunch:
>   if (dev->visorchannel) {
>   visorchannel_destroy(dev->visorchannel);
> 
> yet the first thing that visorchannel_destroy() does is check for null.
> So, no need to test this twice, right, only do so in the function, that
> will make your code flow a lot "smoother" where ever you are calling
> this.
> 
> Ok, I'll stop now, gotta go find some dinner...
> 

We will do some more internal reviews, and send out fixes for things
we find. I hope you enjoyed your dinner.

> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: Solve sparse warning

[Kershner, David A]

> -Original Message-
> From: Marcos Paulo de Souza [mailto:marcos.souza@gmail.com]
> Sent: Tuesday, May 2, 2017 11:28 PM
> Cc: Marcos Paulo de Souza <marcos.souza....@gmail.com>; Kershner, David
> A <david.kersh...@unisys.com>; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; Sell, Timothy C <timothy.s...@unisys.com>;
> Binder, David Anthony <david.bin...@unisys.com>; Erik Arfvidson
> <erik.arfvid...@unisys.com>; Wadgaonkar, Sameer Laxmikant
> <sameer.wadgaon...@unisys.com>; Curtin, Alexander Paul
> <alexander.cur...@unisys.com>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] staging: unisys: Solve sparse warning
> 
> The following commit fixes the following sparse report:
> 
> drivers/staging//unisys/visorhba/visorhba_main.c:660:29: warning: cast to
> restricted __le64
> 
> by casting readq (which is unsigned long on x86) to u64, as expected by the
> seq_printf call.
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza@gmail.com>
> ---
>  Just compiled tested on x86_64.
> 
>  drivers/staging/unisys/visorhba/visorhba_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 6997b16..46d33e6 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -657,7 +657,7 @@ static int info_debugfs_show(struct seq_file *seq,
> void *v)
>   seq_printf(seq, "phys_flags_addr = 0x%016llx\n",
>  phys_flags_addr);
>   seq_printf(seq, "FeatureFlags = %llu\n",
> -(__le64)readq(devdata->flags_addr));
> +(u64)readq(devdata->flags_addr));

Acked-by: David Kershner <david.kersh...@unisys.com>

>   }
>   seq_printf(seq, "acquire_failed_cnt = %llu\n",
>  devdata->acquire_failed_cnt);
> --
> 2.9.3

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 10/22] staging: unisys: visorbus: Make use of the new sg_map helper function

[Kershner, David A]

> -Original Message-
> From: Logan Gunthorpe [mailto:log...@deltatee.com]
...
> Subject: [PATCH 10/22] staging: unisys: visorbus: Make use of the new
> sg_map helper function
> 
> Straightforward conversion to the new function.
> 
> Signed-off-by: Logan Gunthorpe 

Can you add Acked-by for this patch? 

Acked-by: David Kershner 

Tested on s-Par and no problems. 

Thanks,
David Kershner

> ---
>  drivers/staging/unisys/visorhba/visorhba_main.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 0ce92c8..2d8c8bc 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -842,7 +842,6 @@ do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp, struct
> scsi_cmnd *scsicmd)
>   struct scatterlist *sg;
>   unsigned int i;
>   char *this_page;
> - char *this_page_orig;
>   int bufind = 0;
>   struct visordisk_info *vdisk;
>   struct visorhba_devdata *devdata;
> @@ -869,11 +868,14 @@ do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp,
> struct scsi_cmnd *scsicmd)
> 
>   sg = scsi_sglist(scsicmd);
>   for (i = 0; i < scsi_sg_count(scsicmd); i++) {
> - this_page_orig = kmap_atomic(sg_page(sg + i));
> - this_page = (void *)((unsigned long)this_page_orig |
> -  sg[i].offset);
> + this_page = sg_map(sg + i, SG_KMAP_ATOMIC);
> + if (IS_ERR(this_page)) {
> + scsicmd->result = DID_ERROR << 16;
> + return;
> + }
> +
>   memcpy(this_page, buf + bufind, sg[i].length);
> - kunmap_atomic(this_page_orig);
> + sg_unmap(sg + i, this_page, SG_KMAP_ATOMIC);
>   }
>   } else {
>   devdata = (struct visorhba_devdata *)scsidev->host-
> >hostdata;
> --
> 2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] unisys: visornic: Replace symbolic perms with octal

[Kershner, David A]

> -Original Message-
> From: Thomas Jespersen [mailto:laumann.tho...@gmail.com]
> Sent: Tuesday, April 4, 2017 3:15 PM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: gre...@linuxfoundation.org; Sell, Timothy C
> <timothy.s...@unisys.com>; Binder, David Anthony
> <david.bin...@unisys.com>; Frisch, Jon <jon.fri...@unisys.com>;
> erik.arfvid...@unisys.com; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] unisys: visornic: Replace symbolic perms with octal
> 

Should the subject line say "staging: unisys: visornic" instead of just 
"unisys: visornic:"?

Besides that, I'm fine with this patch and have tested it on s-Par. 

David Kershner

> Replace symbolic permissions S_IRUSR and S_IWUSR for their octal
> counterparts
> 
> Signed-off-by: Thomas Jespersen <laumann.tho...@gmail.com>
> ---
>  drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c
> b/drivers/staging/unisys/visornic/visornic_main.c
> index feece91..1008337 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -2146,11 +2146,11 @@ static int visornic_init(void)
>   if (!visornic_debugfs_dir)
>   return err;
> 
> - ret = debugfs_create_file("info", S_IRUSR, visornic_debugfs_dir,
> NULL,
> + ret = debugfs_create_file("info", 0400, visornic_debugfs_dir, NULL,
> _info_fops);
>   if (!ret)
>   goto cleanup_debugfs;
> - ret = debugfs_create_file("enable_ints", S_IWUSR,
> visornic_debugfs_dir,
> + ret = debugfs_create_file("enable_ints", 0200, visornic_debugfs_dir,
> NULL, _enable_ints_fops);
>   if (!ret)
>   goto cleanup_debugfs;
> --
> 2.10.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: fix sparse warnings

[Kershner, David A]

> -Original Message-
> From: Andrea Ghittino [mailto:aghitt...@gmail.com]
> Sent: Saturday, March 4, 2017 12:21 PM
> To: de...@driverdev.osuosl.org; Kershner, David A
> <david.kersh...@unisys.com>; gre...@linuxfoundation.org; *S-Par-
> Maintainer <sparmaintai...@unisys.com>; linux-ker...@vger.kernel.org
> Subject: [PATCH] staging: unisys: fix sparse warnings
> 
> Sparse generates two warnings related to incorrect type in assignment.
> This patch changes the types in the struct defined in unisys
> 
> Signed-off-by: Andrea Ghittino <aghitt...@gmail.com>

Acked-by: David Kershner <david.kersh...@unisys.com>

Tested it on s-Par and no problems. 

> ---
> Compile tested only
> 
>  iochannel.h |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/unisys/include/iochannel.h
> b/drivers/staging/unisys/include/iochannel.h
> index 54f4900..41e5b4e 100644
> --- a/drivers/staging/unisys/include/iochannel.h
> +++ b/drivers/staging/unisys/include/iochannel.h
> @@ -308,8 +308,8 @@ struct net_pkt_xmt {
>   u8 valid;   /* 1 = struct is valid - else ignore */
>   u8 hrawoffv;/* 1 = hwrafoff is valid */
>   u8 nhrawoffv;   /* 1 = nhwrafoff is valid */
> - u16 protocol;   /* specifies packet protocol */
> - u32 csum;   /* value used to set skb->csum at IOPart */
> + __be16 protocol;/* specifies packet protocol */
> + __wsum csum;/* value used to set skb->csum at IOPart */
>   u32 hrawoff;/* value used to set skb->h.raw at IOPart */
>   /* hrawoff points to the start of the TRANSPORT LAYER
> HEADER */
>   u32 nhrawoff;   /* value used to set skb->nh.raw at IOPart */
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [Outreachy kernel] [PATCH v3 0/3] Replace "the the " with "the"

[Kershner, David A]

> -Original Message-
> From: Julia Lawall [mailto:julia.law...@lip6.fr]
> Sent: Friday, March 3, 2017 11:07 AM
> To: simran singhal <singhalsimr...@gmail.com>
> Cc: gre...@linuxfoundation.org; Kershner, David A
> <david.kersh...@unisys.com>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org; larry.fin...@lwfinger.net;
> florian.c.schilha...@googlemail.com; outreachy-ker...@googlegroups.com
> Subject: Re: [Outreachy kernel] [PATCH v3 0/3] Replace "the the " with "the"
> 
> 
> 
> On Fri, 3 Mar 2017, simran singhal wrote:
> 
> > This patch series replace "the the " with "the" in various drivers.
> >
> > v3:
> >   -Resend the complete patch series.
> 
> Acked-by: Julia Lawall <julia.law...@lip6.fr>

Acked-by: David Kershner <david.kersh...@unisys.com>

> 
> >
> >
> > simran singhal (3):
> >   staging: wlan-ng: Replace "the the " with "the"
> >   staging: rtl8192u: Replace "the the " with "the"
> >   staging: unisys: Replace "the the " with "the"
> >
> >  drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 2 +-
> >  drivers/staging/unisys/visorbus/visorbus_main.c   | 2 +-
> >  drivers/staging/wlan-ng/prism2sta.c   | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > --
> > 2.7.4
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> > To post to this group, send email to outreachy-ker...@googlegroups.com.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/1488556746-599-1-
> git-send-email-singhalsimran0%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: visorbus, replace init_timer with setup_timer

[Kershner, David A]

> -Original Message-
> From: Jiri Slaby [mailto:jsl...@suse.cz]
> Sent: Wednesday, February 15, 2017 11:04 AM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: linux-ker...@vger.kernel.org; Stefan Svinciak <xsvi...@fi.muni.cz>; Jiri
> Slaby <jsl...@suse.cz>; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org
> Subject: [PATCH] staging: visorbus, replace init_timer with setup_timer
> 
> From: Stefan Svinciak <xsvi...@fi.muni.cz>
> 
> Newer version is more readable and needs less changes if/when
> timer_struct is to be changed.
> 
> Signed-off-by: Stefan Svinciak <xsvi...@fi.muni.cz>
> Signed-off-by: Jiri Slaby <jsl...@suse.cz>
> Cc: David Kershner <david.kersh...@unisys.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: <sparmaintai...@unisys.com>
> Cc: <de...@driverdev.osuosl.org>

Acked-by: David Kershner <david.kersh...@unisys.com>

Looks good and runs nicely on s-Par. 

David Kershner

> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> index aea1aa262b28..55f29ae8e015 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -623,9 +623,7 @@ create_visor_device(struct visor_device *dev)
>   dev->device.release = visorbus_release_device;
>   /* keep a reference just for us (now 2) */
>   get_device(>device);
> - init_timer(>timer);
> - dev->timer.data = (unsigned long)(dev);
> - dev->timer.function = dev_periodic_work;
> + setup_timer(>timer, dev_periodic_work, (unsigned long)dev);
> 
>   /*
>* bus_id must be a unique name with respect to this bus TYPE
> --
> 2.11.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] [linux-next]staging: unisys: visornic: Fix typo in visornic_main.c

[Kershner, David A]

> -Original Message-
> From: Masanari Iida [mailto:standby2...@gmail.com]
> Sent: Sunday, January 29, 2017 9:33 PM
> To: linux-ker...@vger.kernel.org; gre...@linuxfoundation.org; Kershner,
> David A <david.kersh...@unisys.com>; de...@driverdev.osuosl.org
> Cc: Masanari Iida <standby2...@gmail.com>
> Subject: [PATCH] [linux-next]staging: unisys: visornic: Fix typo in
> visornic_main.c
> 
> This patch fix some spelling typos found in visornic_main.c
> 

Acked-by: David Kershner <david.kersh...@unisys.com> 

> Signed-off-by: Masanari Iida <standby2...@gmail.com>
> ---
>  drivers/staging/unisys/visornic/visornic_main.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c
> b/drivers/staging/unisys/visornic/visornic_main.c
> index 0a8f36125f5b..c44c430b966f 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -423,7 +423,7 @@ send_enbdis(struct net_device *netdev, int state,
> 
>  /**
>   *   visornic_disable_with_timeout - Disable network adapter
> - *   @netdev: netdevice to disale
> + *   @netdev: netdevice to disable
>   *   @timeout: timeout to wait for disable
>   *
>   *   Disable the network adapter and inform the IO Partition that we
> @@ -532,7 +532,7 @@ init_rcv_bufs(struct net_device *netdev, struct
> visornic_devdata *devdata)
>   return -ENOMEM;
>   count = i;
> 
> - /* Ensure we can alloc 2/3rd of the requeested number of buffers.
> + /* Ensure we can alloc 2/3rd of the requested number of buffers.
>* 2/3 is an arbitrary choice; used also in ndis init.c
>*/
>   if (count < ((2 * devdata->num_rcv_bufs) / 3)) {
> @@ -561,7 +561,7 @@ init_rcv_bufs(struct net_device *netdev, struct
> visornic_devdata *devdata)
>   *
>   *   Sends enable to IOVM, inits, and posts receive buffers to IOVM
>   *   timeout is defined in msecs (timeout of 0 specifies infinite wait)
> - *   Return 0 for success, negavite for failure.
> + *   Return 0 for success, negative for failure.
>   */
>  static int
>  visornic_enable_with_timeout(struct net_device *netdev, const int
> timeout)
> @@ -750,7 +750,7 @@ static inline bool vnic_hit_low_watermark(struct
> visornic_devdata *devdata,
>   *   @skb: Packet to be sent
>   *   @netdev: net device the packet is being sent from
>   *
> - *   Convert the skb to a cmdrsp so the IO Partition can undersand it.
> + *   Convert the skb to a cmdrsp so the IO Partition can understand it.
>   *   Send the XMIT command to the IO Partition for processing. This
>   *   function is protected from concurrent calls by a spinlock xmit_lock
>   *   in the net_device struct, but as soon as the function returns it
> @@ -1097,7 +1097,7 @@ repost_return(struct uiscmdrsp *cmdrsp, struct
> visornic_devdata *devdata,
>   *
>   *   Got a receive packet back from the IO Part, handle it and send
>   *   it up the stack.
> - *   Returns 1 iff an skb was receieved, otherwise 0
> + *   Returns 1 iff an skb was received, otherwise 0
>   */
>  static int
>  visornic_rx(struct uiscmdrsp *cmdrsp)
> @@ -1227,7 +1227,7 @@ visornic_rx(struct uiscmdrsp *cmdrsp)
>   }
>   }
> 
> - /* set up packet's protocl type using ethernet header - this
> + /* set up packet's protocol type using ethernet header - this
>* sets up skb->pkt_type & it also PULLS out the eth header
>*/
>   skb->protocol = eth_type_trans(skb, netdev);
> @@ -1549,7 +1549,7 @@ drain_resp_queue(struct uiscmdrsp *cmdrsp,
> struct visornic_devdata *devdata)
>   *   @cmdrsp: io channel command response message
>   *   @devdata: visornic device to drain
>   *
> - *   Drain the respones queue of any responses from the IO partition.
> + *   Drain the response queue of any responses from the IO partition.
>   *   Process the responses as we get them.
>   *   Returns when response queue is empty or when the thread stops.
>   */
> @@ -1665,7 +1665,7 @@ static int visornic_poll(struct napi_struct *napi, int
> budget)
>   *   poll_for_irq- Checks the status of the response queue.
>   *   @v: void pointer to the visronic devdata
>   *
> - *   Main function of the vnic_incoming thread. Peridocially check the
> + *   Main function of the vnic_incoming thread. Periodically check the
>   *   response queue and drain it if needed.
>   *   Returns when thread has stopped.
>   */
> @@ -1711,7 +1711,7 @@ static int visornic_probe(struct visor_device *dev)
>   netdev->watchdog_timeo = 5 * HZ;
>   SET_NETDEV_DEV(netdev, >device);
> 
> - /* 

RE: [PATCH] staging: unisys: fix checkpatch block comments warning

[Kershner, David A]

> -Original Message-
> From: Abdul Rauf Mujahid [mailto:abdulraufmuja...@gmail.com]
> Sent: Tuesday, January 10, 2017 6:51 PM
> To: Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org; j...@redhat.com; *S-Par-Maintainer
> <sparmaintai...@unisys.com>
> Cc: de...@driverdev.osuosl.org
> Subject: Re: [PATCH] staging: unisys: fix checkpatch block comments warning
> 
> 
> 
> On 01/10/2017 06:36 PM, Kershner, David A wrote:
> >> -Original Message-
> >> From: Abdul Rauf [mailto:abdulraufmuja...@gmail.com]
> >> Sent: Tuesday, January 10, 2017 6:24 PM
> >> To: gre...@linuxfoundation.org; j...@redhat.com; *S-Par-Maintainer
> >> <sparmaintai...@unisys.com>
> >> Cc: de...@driverdev.osuosl.org
> >> Subject: [PATCH] staging: unisys: fix checkpatch block comments warning
> > This patch has the same subject line as the previous patch? Which one
> > should we use? Or can you make the names unique?
> >
> > David Kershner
> >
> >> Fix the following warnings:
> >> Block comments should align the * on each line
> >>
> >> Signed-off-by: Abdul Rauf <abdulraufmuja...@gmail.com>
> >> ---
> >>  drivers/staging/unisys/visorbus/visorchipset.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> >> b/drivers/staging/unisys/visorbus/visorchipset.c
> >> index 336af52d43d7..4e630ea527e8 100644
> >> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> >> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> >> @@ -1409,7 +1409,7 @@ parahotplug_process_message(struct
> >> controlvm_message *inmsg)
> >> *
> >> * devices are automatically enabled at
> >> * initialization.
> >> -  */
> >> +   */
> >>parahotplug_request_kickoff(req);
> >>controlvm_respond_physdev_changestate
> >>(>hdr,
> >> --
> >> 2.11.0
> you should use both of them. I am sending both again by changing their
> names.

Thanks!

David Kershner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: fix checkpatch block comments warning

[Kershner, David A]

> -Original Message-
> From: Abdul Rauf [mailto:abdulraufmuja...@gmail.com]
> Sent: Tuesday, January 10, 2017 6:24 PM
> To: gre...@linuxfoundation.org; j...@redhat.com; *S-Par-Maintainer
> 
> Cc: de...@driverdev.osuosl.org
> Subject: [PATCH] staging: unisys: fix checkpatch block comments warning

This patch has the same subject line as the previous patch? Which one
should we use? Or can you make the names unique?

David Kershner

> 
> Fix the following warnings:
> Block comments should align the * on each line
> 
> Signed-off-by: Abdul Rauf 
> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index 336af52d43d7..4e630ea527e8 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -1409,7 +1409,7 @@ parahotplug_process_message(struct
> controlvm_message *inmsg)
>*
>* devices are automatically enabled at
>* initialization.
> - */
> +  */
>   parahotplug_request_kickoff(req);
>   controlvm_respond_physdev_changestate
>   (>hdr,
> --
> 2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 0/2] Staging: unisys: visorbus: style fix, using octal file permissions

[Kershner, David A]

> -Original Message-
> From: Derek Robson [mailto:robso...@gmail.com]
> Sent: Friday, January 6, 2017 10:48 PM
> To: Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org; Sell, Timothy C <timothy.s...@unisys.com>
> Cc: *S-Par-Maintainer <sparmaintai...@unisys.com>;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; Derek Robson
> <robso...@gmail.com>
> Subject: [PATCH 0/2] Staging: unisys: visorbus: style fix, using octal file
> permissions
> 
> Two files change in style fix, changes are octal file permissions.
> 

Series looks fine to me and runs on top of the s-Par firmware.

Acked-by: David Kershner <david.kersh...@unisys.com>

> Derek Robson (2):
>   Staging: unisys: visorbus: style fix, using octal file permissions
>   Staging: unisys: visorbus: style fix, using octal file permissions
> 
>  drivers/staging/unisys/visorbus/visorbus_main.c | 6 +++---
>  drivers/staging/unisys/visorbus/visorchipset.c  | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> --
> 2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: ix spelling mistake of "outstanding"

[Kershner, David A]

> -Original Message-
> From: Colin King [mailto:colin.k...@canonical.com]
> Sent: Tuesday, November 29, 2016 2:07 PM
> To: Kershner, David A <david.kersh...@unisys.com>; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; Sell, Timothy C <timothy.s...@unisys.com>;
> Binder, David Anthony <david.bin...@unisys.com>; Arfvidson, Erik
> <erik.arfvid...@unisys.com>; Frisch, Jon <jon.fri...@unisys.com>; Amitoj
> Kaur Chawla <amitoj1...@gmail.com>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org
> Cc: linux-ker...@vger.kernel.org
> Subject: [PATCH] staging: unisys: ix spelling mistake of "outstanding"
> 

Shouldn't the subjecet say "Fix" instead of fix? 

Besides that I'm fine with the patch.

> From: Colin Ian King <colin.k...@canonical.com>
> 
> Trivial fix to spelling mistake "oustanding" to "outstanding".
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/staging/unisys/visornic/visornic_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c
> b/drivers/staging/unisys/visornic/visornic_main.c
> index f8a584b..c1f674f 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -1371,7 +1371,7 @@ static ssize_t info_debugfs_read(struct file *file,
> char __user *buf,
>" num_rcv_bufs = %d\n",
>devdata->num_rcv_bufs);
>   str_pos += scnprintf(vbuf + str_pos, len - str_pos,
> -  " max_oustanding_next_xmits = %lu\n",
> +  " max_outstanding_next_xmits = %lu\n",
>   devdata->max_outstanding_net_xmits);
>   str_pos += scnprintf(vbuf + str_pos, len - str_pos,
>" upper_threshold_net_xmits = %lu\n",
> --
> 2.10.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v2] staging: unisys: visorbus: visorchannel: Refactor locking code to be statically deterministic.

[Kershner, David A]

> -Original Message-
> From: Cathal Mullaney [mailto:chuckleberryf...@gmail.com]
> Subject: [PATCH v2] staging: unisys: visorbus: visorchannel: Refactor locking
> code to be statically deterministic.
> 
> This patch makes locking in visorchannel_signalempty statically
> deterministic.
> As a result this patch fixes the sparse warning:
> Context imbalance in 'visorchannel_signalempty' - different lock
> contexts for basic block.
> 
> The logic of the locking code doesn't change but the layout of the
> original code is "frowned upon"
> according to mails on sparse context checking.
> Refactoring removes the warning and makes the code more readable.
> 
> Signed-off-by: Cathal Mullaney 

Tested-by: David Kershner 

> ---
> V2: Removed unnecessary variable initialization, as suggested by Tim Sell
> .
> 
>  drivers/staging/unisys/visorbus/visorchannel.c | 30 
> --

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 00/21] Clean up header files and remove unused structures

[Kershner, David A]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, September 1, 2016 12:25 PM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: driverdev-devel@linuxdriverproject.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; jes.soren...@redhat.com
> Subject: Re: [PATCH 00/21] Clean up header files and remove unused
> structures
> 
> On Thu, Sep 01, 2016 at 11:17:57AM -0400, David Kershner wrote:
> > This patch series removes unused defines and structures in visorbus.
> >
> > It also combines and simplifies the headers files for the s-Par
> > drivers.
> 
> Given that I pointed out almost all of these issues, it's usually a nice
> thing to add a:
>   Reported-by: Some Developers <email_g...@here.com>
> 
> Can you respin this series with that information?  Otherwise people are
> less likely to want to do code reviews for you, if you don't give
> credit...
> 


Sorry about that, will do! 

Thanks,
David Kershner


> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH RESEND 00/28] staging: unisys: fix visorbus & visorinput issues raised by tglx

[Kershner, David A]

> -Original Message-
> From: gre...@linuxfoundation.org [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, June 16, 2016 3:03 PM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: Neil Horman <nhor...@redhat.com>; driverdev-
> de...@linuxdriverproject.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; jes.soren...@redhat.com;
> t...@linutronix.de; Binder, David Anthony <david.bin...@unisys.com>
> Subject: Re: [PATCH RESEND 00/28] staging: unisys: fix visorbus & visorinput
> issues raised by tglx
> 
> On Thu, Jun 16, 2016 at 05:35:58PM +, Kershner, David A wrote:
> >
> >
...
> > Was wondering what the status of this patch series was and if you
> > need anything from us?
> 
> Again, staging patches are at the bottom of my queue, you are behind 500
> other staging patches, 275 serial patches, 100 USB patches, and 50+
> other char/misc patches, please be patient.
> 

I understand that the 4.8 window is currently open, and was wondering if we
are still in the queue or if we need to resend the patch series.

Thanks,
David Kershner

> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH RESEND 00/28] staging: unisys: fix visorbus & visorinput issues raised by tglx

[Kershner, David A]

> -Original Message-
> From: Neil Horman [mailto:nhor...@redhat.com]
> Sent: Tuesday, June 14, 2016 9:27 AM
> Subject: Re: [PATCH RESEND 00/28] staging: unisys: fix visorbus & visorinput
> issues raised by tglx
> 
> On Fri, Jun 10, 2016 at 09:47:58PM -0400, David Kershner wrote:
> > Resending due to imcomplete distribution list.
> >
> > This patchset comprises the first 26 patches of the previously-submitted
> > patchset (but retracted):
> >
> > [PATCH v4 00/29] Fixed issues raised by tglx, then move visorbus to
> >  drivers/virt
> >
> > then adds 2 patches to visorinput that:
> > * fixes a device initialization race condition
> > * converts a semaphore to a mutex
> >
> > As described in the email NAKing the previously-submitted patchset,
> > the reason we are re-submitting this now is to make things a bit
> > cleaner by separating the fixes we need to make to the code in
> > staging from the patchset that actually moves the code out of staging.
> >
> > The intent of this patchset is to fix all known outstanding
> > issues with code in drivers/staging/unisys/, so that subsequent
> > patchsets can move these drivers out of staging.
> >
> > tglx: The following patchset fixes issues you raised during your
> > code review of visorbus on 5/18, and visorinput on 6/1.
> >
...
> > --
> > 1.9.1
> >
> Series
> Acked-By: Neil Horman 

Was wondering what the status of this patch series was and if you
need anything from us?

Thanks,
David Kershner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[NAK] [PATCH v4 00/29] Fixed issues raised by tglx, then move visorbus to drivers/virt

[Kershner, David A]

> -Original Message-
> From: David Kershner [mailto:david.kersh...@unisys.com]
> Sent: Wednesday, June 8, 2016 5:14 PM
> To: cor...@lwn.net; t...@linutronix.de; mi...@redhat.com;
> h...@zytor.com; Kershner, David A <david.kersh...@unisys.com>;
> gre...@linuxfoundation.org; Arfvidson, Erik <erik.arfvid...@unisys.com>;
> Sell, Timothy C <timothy.s...@unisys.com>; hof...@osadl.org;
> dzic...@redhat.com; jes.soren...@redhat.com; Curtin, Alexander Paul
> <alexander.cur...@unisys.com>; janani.rvchn...@gmail.com;
> sudipm.mukher...@gmail.com; pra...@redhat.com; Binder, David Anthony
> <david.bin...@unisys.com>; nhor...@redhat.com;
> dan.j.willi...@intel.com; linux-ker...@vger.kernel.org; linux-
> d...@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer <sparmaintai...@unisys.com>
> Subject: [PATCH v4 00/29] Fixed issues raised by tglx, then move visorbus to
> drivers/virt
>

NAKed-by: David Kershner <david.kersh...@unisys.com>

We have decided that it would be cleaner if we separated this patchset
into 2 smaller patchsets:
 * patchset 1: fixes we need to make to the drivers in staging that
are required before they are clean enough to move out of staging
 * patchset 2: patches that move code out of staging

Hence, we are retracting this patchset, and will soon submit 2 follow-on
patchsets that are split as-per the above bullets.


> tglx: The following patchset fixes issues you raised during your
> code review of visorbus on 5/18.
> 
> Converts visorbus to use a kernel timer for periodic device-specific
> callbacks instead of a workqueue, making the implementation in
> periodic_work.c and periodic_work.h no longer necessary.  These files
> are then deleted.
> 
> The visordriver_callback_lock has been switched to a mutex.
> 
> Several module parameters and structures were removed that were no
> longer being used.
> 
> Moves drivers/staging/unisys/include to include/linux/visorbus
> and moves drivers/staging/unisys/visorbus to drivers/virt/visorbus.
> 
> Changes since v3:
>  - Updated patches to apply cleanly on top of staging-next
>  - Modified patch comment to point to correct destination directory
> 
> Changes since v2:
>  - Fixed comments raised by Neil Horman regarding return -1 changes
>  - Fixed kernel-doc style comments for visorbus driver, and added
>kernel-doc documentation for many more functions
>  - Reworked visorbus so we did not need notifier_lock
> 
> Changes since v1:
>  - Added the patch staging: unisys: visorbus change -1 return values
>  - Added the patch staging: unisys: visorchipset change -1 return value
>  - Added the patch staging: unisys: iovmcall_gnuc.h change -1 return values
> 
> Bryan Thompson (4):
>   staging: unisys: visorbus: Make visordriver_callback_lock a mutex
>   staging: unisys: visorbus: Remove unnecessary EXPORT_SYMBOL
> statements
>   staging: unisys: visorbus: Remove unused functions
>   staging: unisys: Remove reference to unused STANDALONE_CLIENT
> 
> David Binder (13):
>   staging: unisys: visorbus: remove unused module parameters
>   staging: unisys: visorbus: remove unused struct
>   staging: unisys: visorbus: modify format string to match argument
>   staging: unisys: visornic: Correct comment spelling mistake
>   staging: unisys: include: Remove thread-related enum members
>   staging: unisys: visorbus: fix commenting in vbusdevinfo.h
>   staging: unisys: visorbus: fix commenting in visorbus_main.c
>   staging: unisys: visorbus: fix visorchannel.c comments
>   staging: unisys: visorbus: Rectify commenting in visorchipset.c
>   staging: unisys: visorbus: Move visorbus-unique functions to private
> header
>   staging: unisys: visorbus: rectify kerneldoc comment for struct
>   staging: unisys: visorbus: Remove notifier-related code from visorbus
>   staging: unisys: visorbus: Rename function to follow existing
> convention
> 
> David Kershner (4):
>   staging: unisys: Move vbushelper.h to visorbus directory
>   include: linux: visorbus: Add visorbus to include/linux directory
>   Documentation: Move visorbus documentation from staging to
> Documentation/
>   drivers: Add visorbus to the drivers/virt directory
> 
> Erik Arfvidson (2):
>   staging: unisys: visorbus: remove return values for write_vbus
> functions
>   staging: unisys: visorbus: check parahotplug_request_complete_result
> 
> Tim Sell (6):
>   staging: unisys: visorbus: removed unused periodic_test_workqueue
>   staging: unisys: visorinput: remove unnecessary locking
>   staging: unisys: visorbus: use kernel timer instead of workqueue
>   staging: unisys: visorbus: remove periodic_work.h/.c
>   staging: unisys: visorbus: remove unused p

RE: [PATCH] staging: unisys: fix format string %Lx to %llx for u64

[Kershner, David A]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, May 5, 2016 9:46 PM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: driverdev-devel@linuxdriverproject.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; Arfvidson, Erik
> <erik.arfvid...@unisys.com>
> Subject: Re: [PATCH] staging: unisys: fix format string %Lx to %llx for u64
> 
> On Thu, May 05, 2016 at 10:25:59PM -0400, David Kershner wrote:
> > From: Erik Arfvidson <erik.arfvid...@unisys.com>
> >
> > this patch fixes the following sonarqube issue.
> > %Lx in format string (no. 1) requires 'unsigned long long'
> > but the argument type is 'unsigned long long'
> >
> > Signed-off-by: Erik Arfvidson <erik.arfvid...@unisys.com>
> > Signed-off-by: David Kershner <david.kersh...@unisys.com>
> > ---
> >  drivers/staging/unisys/visorbus/visorbus_main.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> 
> Please send patches as series, otherwise I have no idea what order to
> apply them, so I'll just give up and delete them all from my queue.
> 
> You know better than this :(
> 

Sorry, didn't send as a series cause they can be applied independently of
each other. Should I consider all the patches sent dropped and send as a
series this time?

Thanks, 
David Kershner

> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: Time for a code audit?

[Kershner, David A]

> -Original Message-
> From: Romer, Benjamin M
> Sent: Tuesday, February 16, 2016 10:03 AM
> To: Dan Carpenter 
> Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> Sell, Timothy C ; *S-Par-Maintainer
> 
> Subject: Re: Time for a code audit?
> 
...
> 
> Hi Dan,
> 
> Thank you for this list! We'll get started on fixing these issues
> immediately. I've run Smatch using the make options
> 
> O=../builds/testbuild CC=gcc-4.9 CHECK="smatch -p=kernel" C=1
> 
> but when I use it this way, I don't get that line where it complains
> about the variable's size. What settings do you use?
> 
> If I can generate a list of problems that way across the whole driver
> set, we can just resolve them all without having to bother you or Greg
> until we have patches for them.
> 
> Thanks a ton for the review. :)
> 
> -- Ben

Greg, 

With the patch "[PATCH] staging: unisys: visorbus: initialize variables",
I believe all the issues brought up by both you and Dan are addressed by
the patches you accepted. Also during the time we did some additional
cleanup.

What is the next step to get visorbus out of the staging directory? What
do we do with the include files?

Also, I'm working on removing the MODULE_ALIAS lines from visorinput,
visornic, and visorhba. I believe this requires me to make changes in
include/kernel/mod_devicemodules.h and scripts/mod/*. Should I
send those fixes to the respective maintainers now or when we move
the other drivers from staging?

Thanks,
David Kershner


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: visorbus: initialize variables

[Kershner, David A]

> -Original Message-
> From: David Kershner [mailto:david.kersh...@unisys.com]
> Sent: Monday, April 18, 2016 10:40 PM
> To: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> *S-Par-Maintainer <sparmaintai...@unisys.com>
> Cc: Binder, David Anthony <david.bin...@unisys.com>; Kershner, David A
> <david.kersh...@unisys.com>
> Subject: [PATCH] staging: unisys: visorbus: initialize variables
> 
> From: David Binder <david.bin...@unisys.com>
> 
> Initializes previously uninitialized variables that were flagged
> as being problematic by Smatch.
> 
> Signed-off-by: David Binder <david.bin...@unisys.com>
> Signed-off-by: David Kershner <david.kersh...@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index c1b872c..ce2a80e 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -512,7 +512,7 @@ static ssize_t toolaction_show(struct device *dev,
>  struct device_attribute *attr,
>  char *buf)
>  {
> - u8 tool_action;
> + u8 tool_action = 0;
> 
>   visorchannel_read(controlvm_channel,
> offsetof(struct spar_controlvm_channel_protocol,
> @@ -580,7 +580,7 @@ static ssize_t boottotool_store(struct device *dev,
>  static ssize_t error_show(struct device *dev, struct device_attribute *attr,
> char *buf)
>  {
> - u32 error;
> + u32 error = 0;
> 
>   visorchannel_read(controlvm_channel,
> offsetof(struct spar_controlvm_channel_protocol,
> @@ -611,7 +611,7 @@ static ssize_t error_store(struct device *dev, struct
> device_attribute *attr,
>  static ssize_t textid_show(struct device *dev, struct device_attribute *attr,
>  char *buf)
>  {
> - u32 text_id;
> + u32 text_id = 0;
> 
>   visorchannel_read
>   (controlvm_channel,
> @@ -643,7 +643,7 @@ static ssize_t textid_store(struct device *dev, struct
> device_attribute *attr,
>  static ssize_t remaining_steps_show(struct device *dev,
>   struct device_attribute *attr, char *buf)
>  {
> - u16 remaining_steps;
> + u16 remaining_steps = 0;
> 
>   visorchannel_read(controlvm_channel,
> offsetof(struct spar_controlvm_channel_protocol,
> --
> 1.9.1

Greg, 

I was wondering what the status of this patch is? Has it been accepted or does 
it still need some work?

Thanks!
David Kershner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: When and how to commit kerneldoc comments to staging/unisys/*

[Kershner, David A]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Tuesday, March 22, 2016 11:59 AM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: driverdev-devel@linuxdriverproject.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>
> Subject: Re: When and how to commit kerneldoc comments to
> staging/unisys/*
> 
> On Tue, Mar 22, 2016 at 03:51:59PM +, Kershner, David A wrote:
> > Greg, we are looking at adding kerneldoc style comments for the drivers in
> staging/unisys/*.
> 
> First off, why?

Cause the current state of the documentation in the tree has something to be
desired and is extremely lacking. I just picked kerneldoc style cause it is
definitely better than what we currently have, should we use some other style
guideline?
> 
> > Should we be doing this in staging or outside of it?
> 
> I don't understand what you mean by "outside of it"?

Is the documentation we have in the code good enough for the drivers to move
beyond the staging directory or should they get updated? 

> 
> > If we should, do we have one commit per file or one commit per function?
> 
> What do you think would be the easiest to review if you were on the
> receiving end of such changes?

Understood. 
 
Thanks,
David Kershner
 
> 
> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

When and how to commit kerneldoc comments to staging/unisys/*

[Kershner, David A]

Greg, we are looking at adding kerneldoc style comments for the drivers in 
staging/unisys/*.

Should we be doing this in staging or outside of it? 

If we should, do we have one commit per file or one commit per function?

Thanks,
David Kershner

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [RFC] staging: unisys: visornic: Remove create_singlethread_workqueue()

[Kershner, David A]

> -Original Message-
> From: Amitoj Kaur Chawla [mailto:amitoj1...@gmail.com]
> Sent: Sunday, February 28, 2016 7:57 AM
> To: Romer, Benjamin M <benjamin.ro...@unisys.com>;
> jes.soren...@redhat.com; Arfvidson, Erik <erik.arfvid...@unisys.com>;
> Kershner, David A <david.kersh...@unisys.com>; j...@redhat.com; Sell,
> Timothy C <timothy.s...@unisys.com>; Thompson, Bryan E.
> <bryan.thomp...@unisys.com>; dzic...@redhat.com;
> gre...@linuxfoundation.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Cc: t...@kernel.org; outreachy-ker...@googlegroups.com
> Subject: [RFC] staging: unisys: visornic: Remove
> create_singlethread_workqueue()
> 
> With concurrency managed workqueues, use of dedicated workqueues
> can be replaced by using system_wq.
> Drop visornic_timeout_reset_workqueue by using system_wq.
> 
> Since there is only one work item per devdata and different
> devdatas do not need to be ordered, increase of concurrency
> level by switching to system_wq should not break anything.
> 
> cancel_work_sync() is used to ensure that work is not pending or
> executing on any CPU.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1...@gmail.com>
> Acked-by: Tejun Heo <t...@kernel.org>

Tested on s-Par system, no issues found. 

Tested-by: David Kershner <david.kersh...@unisys.com>

> ---
> Only compile tested.
> 
>  drivers/staging/unisys/visornic/visornic_main.c | 21 ++---
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c
> b/drivers/staging/unisys/visornic/visornic_main.c
> index df4f688..6749c4e 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -59,8 +59,6 @@ static const struct file_operations
> debugfs_enable_ints_fops = {
>   .write = enable_ints_write,
>  };
> 
> -static struct workqueue_struct *visornic_timeout_reset_workqueue;
> -
>  /* GUIDS for director channel type supported by this driver.  */
>  static struct visor_channeltype_descriptor visornic_channel_types[] = {
>   /* Note that the only channel type we expect to be reported by the
> @@ -1070,7 +1068,7 @@ visornic_xmit_timeout(struct net_device *netdev)
>   spin_unlock_irqrestore(>priv_lock, flags);
>   return;
>   }
> - queue_work(visornic_timeout_reset_workqueue, 
> >timeout_reset);
> + schedule_work(>timeout_reset);
>   spin_unlock_irqrestore(>priv_lock, flags);
>  }
> 
> @@ -1998,7 +1996,7 @@ static void visornic_remove(struct visor_device
> *dev)
>   }
> 
>   /* going_away prevents new items being added to the workqueues
> */
> - flush_workqueue(visornic_timeout_reset_workqueue);
> + cancel_work_sync(>timeout_reset);
> 
>   debugfs_remove_recursive(devdata->eth_debugfs_dir);
> 
> @@ -2117,21 +2115,10 @@ static int visornic_init(void)
>   if (!ret)
>   goto cleanup_debugfs;
> 
> - /* create workqueue for tx timeout reset */
> - visornic_timeout_reset_workqueue =
> - create_singlethread_workqueue("visornic_timeout_reset");
> - if (!visornic_timeout_reset_workqueue)
> - goto cleanup_workqueue;
> -
>   err = visorbus_register_visor_driver(_driver);
>   if (!err)
>   return 0;
> 
> -cleanup_workqueue:
> - if (visornic_timeout_reset_workqueue) {
> - flush_workqueue(visornic_timeout_reset_workqueue);
> - destroy_workqueue(visornic_timeout_reset_workqueue);
> - }
>  cleanup_debugfs:
>   debugfs_remove_recursive(visornic_debugfs_dir);
> 
> @@ -2147,10 +2134,6 @@ static void visornic_cleanup(void)
>  {
>   visorbus_unregister_visor_driver(_driver);
> 
> - if (visornic_timeout_reset_workqueue) {
> - flush_workqueue(visornic_timeout_reset_workqueue);
> - destroy_workqueue(visornic_timeout_reset_workqueue);
> - }
>   debugfs_remove_recursive(visornic_debugfs_dir);
>  }
> 
> --
> 1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [RFC] staging: unisys: visorbus: visorchipset: Remove create_singlethread_workqueue()

[Kershner, David A]

> -Original Message-
> From: Amitoj Kaur Chawla [mailto:amitoj1...@gmail.com]
> Sent: Sunday, February 28, 2016 7:44 AM
> To: Romer, Benjamin M <benjamin.ro...@unisys.com>;
> jes.soren...@redhat.com; Arfvidson, Erik <erik.arfvid...@unisys.com>;
> Kershner, David A <david.kersh...@unisys.com>; j...@redhat.com; Sell,
> Timothy C <timothy.s...@unisys.com>; Thompson, Bryan E.
> <bryan.thomp...@unisys.com>; dzic...@redhat.com;
> kenneth.de...@unisys.com; gre...@linuxfoundation.org; *S-Par-
> Maintainer <sparmaintai...@unisys.com>; de...@driverdev.osuosl.org;
> linux-ker...@vger.kernel.org
> Cc: t...@kernel.org; outreachy-ker...@googlegroups.com
> Subject: [RFC] staging: unisys: visorbus: visorchipset: Remove
> create_singlethread_workqueue()
> 
> With concurrency managed workqueues, use of dedicated workqueues
> can be replaced by using system_wq. Drop periodic_controlvm_workqueue
> by using system_wq.
> 
> Since there is only one work item periodic_controlvm_work and
> different periodic_controlvm_works do not need to be ordered, increase
> of concurrency level by switching to system_wq should not break anything.
> 
> cancel_delayed_work_sync() is used to ensure that work is not pending
> or executing on any CPU.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1...@gmail.com>
> Acked-by: Tejun Heo <t...@kernel.org>

Tested on s-Par system, no issues found. 

Tested-by: David Kershner <david.kersh...@unisys.com>

> ---
> Only compile tested.
> 
>  drivers/staging/unisys/visorbus/visorchipset.c | 23 ---
>  1 file changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index b75b063..c4c71c6 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -102,7 +102,6 @@ struct parser_context {
>  };
> 
>  static struct delayed_work periodic_controlvm_work;
> -static struct workqueue_struct *periodic_controlvm_workqueue;
>  static DEFINE_SEMAPHORE(notifier_lock);
> 
>  static struct cdev file_cdev;
> @@ -1913,8 +1912,7 @@ cleanup:
>   poll_jiffies =
> POLLJIFFIES_CONTROLVMCHANNEL_FAST;
>   }
> 
> - queue_delayed_work(periodic_controlvm_workqueue,
> -_controlvm_work, poll_jiffies);
> + schedule_delayed_work(_controlvm_work, poll_jiffies);
>  }
> 
>  static void
> @@ -2011,8 +2009,7 @@ cleanup:
> 
>   poll_jiffies = POLLJIFFIES_CONTROLVMCHANNEL_SLOW;
> 
> - queue_delayed_work(periodic_controlvm_workqueue,
> -_controlvm_work, poll_jiffies);
> + schedule_delayed_work(_controlvm_work, poll_jiffies);
>  }
> 
>  static void
> @@ -2299,19 +2296,10 @@ visorchipset_init(struct acpi_device
> *acpi_device)
>   else
>   INIT_DELAYED_WORK(_controlvm_work,
> controlvm_periodic_work);
> - periodic_controlvm_workqueue =
> - create_singlethread_workqueue("visorchipset_controlvm");
> 
> - if (!periodic_controlvm_workqueue) {
> - POSTCODE_LINUX_2(CREATE_WORKQUEUE_FAILED_PC,
> -  DIAG_SEVERITY_ERR);
> - rc = -ENOMEM;
> - goto cleanup;
> - }
>   most_recent_message_jiffies = jiffies;
>   poll_jiffies = POLLJIFFIES_CONTROLVMCHANNEL_FAST;
> - queue_delayed_work(periodic_controlvm_workqueue,
> -_controlvm_work, poll_jiffies);
> + schedule_delayed_work(_controlvm_work, poll_jiffies);
> 
>   visorchipset_platform_device.dev.devt = major_dev;
>   if (platform_device_register(_platform_device) < 0) {
> @@ -2346,10 +2334,7 @@ visorchipset_exit(struct acpi_device *acpi_device)
> 
>   visorbus_exit();
> 
> - cancel_delayed_work(_controlvm_work);
> - flush_workqueue(periodic_controlvm_workqueue);
> - destroy_workqueue(periodic_controlvm_workqueue);
> - periodic_controlvm_workqueue = NULL;
> + cancel_delayed_work_sync(_controlvm_work);
>   destroy_controlvm_payload_info(_payload_info);
> 
>   memset(_chipset_msg_hdr, 0, sizeof(struct
> controlvm_message_header));
> --
> 1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: Time for a code audit?

[Kershner, David A]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Tuesday, February 23, 2016 3:26 PM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: Romer, Benjamin M <benjamin.ro...@unisys.com>;
> gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; Sell,
> Timothy C <timothy.s...@unisys.com>; *S-Par-Maintainer
> <sparmaintai...@unisys.com>
> Subject: Re: Time for a code audit?
> 
> On Tue, Feb 23, 2016 at 05:45:55PM +, Kershner, David A wrote:
> >
> >
> > > -Original Message-
> > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> > > Sent: Friday, February 12, 2016 5:02 PM
> > > To: Romer, Benjamin M <benjamin.ro...@unisys.com>
> > > Cc: gre...@linuxfoundation.org; driverdev-
> de...@linuxdriverproject.org;
> > > Sell, Timothy C <timothy.s...@unisys.com>; *S-Par-Maintainer
> > > <sparmaintai...@unisys.com>
> > > Subject: Re: Time for a code audit?
> > >
> > > I looked at the Smatch warnings, plus some bonus stuff I'm still working
> > > on.
> > >
> > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries()
> > > warn: inconsistent indenting
> > > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries()
> > > warn: inconsistent indenting
> > > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries()
> > > warn: XXX should 'inp_pfn + i' be a 64 bit type?
> > >
> > > This is because the left side is u64 but we wrap at u32.
> > >
> >
> > Dan, thanks for the find. Though I'm curious what options are you using
> when you run smatch to produce the "warn: XXX should 'inp_pfn +I' be a 64
> bit type" message? When I run smatch locally I don't see that message but I
> see the other two and it is definitely a problem in the codebase that I'm
> looking at.
> >
> 
> These are things I'm still working on and haven't published yet.  It
> feels like it should be a warning though.  I'm not certain the heuristic
> to use here.  Sometimes people declare things as u64 when they don't
> care.  Also Smatch tries to be clever about if the integer overflow is
> actually possible but sometimes it still gets false positives.
> 
> regards,
> dan carpenter

Is it possible for me to get a copy of your patches so I can verify that my fix 
doesn't break anything else?

David 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: Time for a code audit?

[Kershner, David A]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Friday, February 12, 2016 5:02 PM
> To: Romer, Benjamin M 
> Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> Sell, Timothy C ; *S-Par-Maintainer
> 
> Subject: Re: Time for a code audit?
> 
> I looked at the Smatch warnings, plus some bonus stuff I'm still working
> on.
> 
> drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries()
> warn: inconsistent indenting
> drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries()
> warn: inconsistent indenting
> drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries()
> warn: XXX should 'inp_pfn + i' be a 64 bit type?
> 
> This is because the left side is u64 but we wrap at u32.
> 

Dan, thanks for the find. Though I'm curious what options are you using when 
you run smatch to produce the "warn: XXX should 'inp_pfn +I' be a 64 bit type" 
message? When I run smatch locally I don't see that message but I see the other 
two and it is definitely a problem in the codebase that I'm looking at. 

David 

> drivers/staging/unisys/visorbus/visorbus_main.c:787
> visordriver_probe_device() warn: 'sem:>visordriver_callback_lock' is
> sometimes locked here and sometimes unlocked.
> drivers/staging/unisys/visorbus/visorchipset.c:542 toolaction_show() warn:
> XXX passing uninitialized 'tool_action'
> drivers/staging/unisys/visorbus/visorchipset.c:609 error_show() warn: XXX
> passing uninitialized 'error'
> drivers/staging/unisys/visorbus/visorchipset.c:639 textid_show() warn: XXX
> passing uninitialized 'text_id'
> drivers/staging/unisys/visorbus/visorchipset.c:669 remaining_steps_show()
> warn: XXX passing uninitialized 'remaining_steps'
> 
> These with XXX are if channel->nbytes is too small.
> 
> drivers/staging/unisys/visorbus/visorchipset.c:2217 visorchipset_ioctl() warn:
> user controlled 'adjustment' cast to postive rl = 's64min-s64max'
> 
> This is because we read a s64 and pass it to a function as u64.
> 
> Do an:  grep "rc = -1" drivers/staging/unisys/ -R
> 
> Also "if (rc != 0)" is a double negative.  != zero is good style when
> you are talking about the number zero or for strcmp() functions.
> 
> Ho ho ho.
> int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev-
> >devnodes[0]);
> 
> You guys know that when I read
> drivers/staging/unisys/visorbus/visorbus_main.c there is still a lot
> that makes me itch.  All the unnecessary initialization to -1 and the
> goto aways.
> 
> char s[99];  Why 99?  Why s?  I'm not a fan of a lot of the naming.
> What is s?  What is x?
> 
> Blank lines after declaration sections.
> 
>781  fix_vbus_dev_info(dev);
>782  up(>visordriver_callback_lock);
>783  rc = 0;
>784  away:
>785  if (rc != 0)
>786  put_device(>device);
>787  return rc;
>788  }
> 
> This is like in my kitchen because when I'm making spaghetti I throw
> some against the wall to see if it is done and eventual the whole wall
> has tiny spots of pasta stuck to it.
> 
> I'm half way through the first file and this code is *almost* so close
> to being ready, but could you just go through it once more and do a
> final tidy up.
> 
> regards,
> dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 01/15] staging: unisys: Remove num_visornic_open array

[Kershner, David A]

We don't have any scripts that access this debugfs file.

David 

 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Tuesday, July 21, 2015 10:05 AM
 To: Romer, Benjamin M
 Cc: gre...@linuxfoundation.org; jes.soren...@redhat.com; *S-Par-
 Maintainer; driverdev-devel@linuxdriverproject.org; nhor...@redhat.com
 Subject: Re: [PATCH 01/15] staging: unisys: Remove num_visornic_open
 array
 
 On Tue, Jul 21, 2015 at 09:55:35AM -0400, Benjamin Romer wrote:
   static ssize_t enable_ints_write(struct file *file,
   const char __user *buffer,
   size_t count, loff_t *ppos)
   {
  -   char buf[4];
  -   int i, new_value;
  -   struct visornic_devdata *devdata;
  -
  -   if (count = ARRAY_SIZE(buf))
  -   return -EINVAL;
  -
  -   buf[count] = '\0';
  -   if (copy_from_user(buf, buffer, count))
  -   return -EFAULT;
  -
  -   i = kstrtoint(buf, 10, new_value);
  -   if (i != 0)
  -   return -EFAULT;
  -
  -   /* set all counts to new_value usually 0 */
  -   for (i = 0; i  VISORNICSOPENMAX; i++) {
  -   if (num_visornic_open[i]) {
  -   devdata = netdev_priv(num_visornic_open[i]);
  -   /* TODO update features bit in channel */
  -   }
  -   }
  -
  +   /*
  +* Don't want to break ABI here by having a debugfs
  +* file that no longer exists or is writable, so
  +* lets just make this a vestigual function
  +*/
  return count;
   }
 
 Are there scripts which will break if we just remove the file?
 Userspace does all kinds of silly things but does it really rely on a
 writeable debugfs?
 
 regards,
 dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: unisys: Add s-Par visorhba

[Kershner, David A]

 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Wednesday, July 15, 2015 8:16 AM
 To: Romer, Benjamin M
 Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
 jes.soren...@redhat.com; *S-Par-Maintainer
 Subject: Re: [PATCH] staging: unisys: Add s-Par visorhba
 
 Since you are redoing this anyway...
 
 On Mon, Jul 13, 2015 at 02:38:23PM -0400, Benjamin Romer wrote:
  +   for (vdisk = devdata-head; vdisk-next; vdisk = vdisk-next) {
  +   if ((scsidev-channel == vdisk-channel) 
  +   (scsidev-id == vdisk-id) 
  +   (scsidev-lun == vdisk-lun)) {
  +   if (atomic_read(vdisk-error_count) 
  +   VISORHBA_ERROR_COUNT)
  +   atomic_inc(vdisk-error_count);
  +   else
  +   atomic_set(vdisk-ios_threshold,
  +  IOS_ERROR_THRESHOLD);
  +   }
  +   }
 
 
 We do this loop all the time, and we're hitting the 80 character
 limit.  Make it a define.
 
 #define for_each_vdisk_match(iter, list, match) \
   for (iter = list-head; iter-next; iter = iter-next) \
   if (iter-channel == match-channel   \
   iter-id == match-id \
   iter-lun == match-lun)
 
 Btw, avoid using too many parenthesis.  It makes the code harder to read
 and it silences GCC's check for == vs = typos so it can lead to bugs.
 
 Now the loop looks like:
 
   for_each_vdisk_match(vdisk, devdata, scsidev) {
   if (atomic_read(vdisk-error_count) 
 VISORHBA_ERROR_COUNT)
   atomic_inc(vdisk-error_count);
   else
   atomic_set(vdisk-ios_threshold,
 IOS_ERROR_THRESHOLD);
 
   }
 
 (Written in email client.  Caveat emptor.)

Thanks for the comments and I really like this idea. When I put it in the code, 
I get the following from checkpatch: 

ERROR: Macros with complex values should be enclosed in parentheses
#31: FILE: drivers/staging/unisys/visorhba/visorhba_main.c:157:
+#define for_each_vdisk_match(iter, list, match)  \
+   for (iter = list-head; iter-next; iter = iter-next) \
+   if (iter-channel == match-channel \
+   iter-id == match-id   \
+   iter-lun == match-lun)

total: 1 errors, 0 warnings, 0 checks, 275 lines checked

Your patch has style problems, please review.

Any ideas what needs to be wrapped to resolved the checkpatch error?

 
  +static int
  +visorhba_queue_command_lck(struct scsi_cmnd *scsicmd,
  +  void (*visorhba_cmnd_done)(struct scsi_cmnd *))
  +{
  +   struct scsi_device *scsidev = scsicmd-device;
  +   int insert_location;
  +   unsigned char op;
  +   unsigned char *cdb = scsicmd-cmnd;
  +   struct Scsi_Host *scsihost = scsidev-host;
  +   struct uiscmdrsp *cmdrsp;
  +   unsigned int i;
  +   struct visorhba_devdata *devdata =
  +   (struct visorhba_devdata *)scsihost-hostdata;
  +   struct scatterlist *sg = NULL;
  +   struct scatterlist *sglist = NULL;
  +   int err = 0;
  +
  +   if (devdata-serverdown || devdata-serverchangingstate)
  +   return SCSI_MLQUEUE_DEVICE_BUSY;
  +
  +   cmdrsp = kzalloc(sizeof(*cmdrsp), GFP_ATOMIC);
  +   if (!cmdrsp)
  +   return -ENOMEM;
  +
  +   /* now saving everything we need from scsi_cmd into cmdrsp
  +* before we queue cmdrsp set type to command - as opposed to
  +* task mgmt
  +*/
  +   cmdrsp-cmdtype = CMD_SCSI_TYPE;
  +   /* save the pending insertion location. Deletion from pending
  +* will return the scsicmd pointer for completion
  +*/
  +   insert_location =
  +   add_scsipending_entry(devdata, CMD_SCSI_TYPE, (void
 *)scsicmd);
  +   if (insert_location != -1) {
  +   cmdrsp-scsi.scsicmd = (void *)(uintptr_t)insert_location;
  +   } else {
  +   kfree(cmdrsp);
 
 This kfree in the middle of the function is weird.
 
  +   return SCSI_MLQUEUE_DEVICE_BUSY;
  +   }
 
 The Spar driver tends to have one error label on the end of each
 function and it has had very buggy error handling...  I wrote a google
 plus post on how to do error handling.
 
 https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
 
 Instead of trying to match the existing buggy style, just adopt normal
 kernel style.
 
 regards,
 dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v2 2/3] staging: unisys: convert pack pragma to __packed

[Kershner, David A]

 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Friday, June 19, 2015 8:55 AM
 To: Kershner, David A
 Cc: gre...@linuxfoundation.org; jes.soren...@redhat.com; *S-Par-
 Maintainer; driverdev-devel@linuxdriverproject.org; Romer, Benjamin M
 Subject: Re: [PATCH v2 2/3] staging: unisys: convert pack pragma to
 __packed
 
 On Fri, Jun 12, 2015 at 04:46:07PM -0400, David Kershner wrote:
  It was noticed that iochannel.h was still using pragmas to
  pack the datastructures, should be using __packed instead.
 
  Signed-off-by: David Kershner david.kersh...@unisys.com
 
 Could you take some time and figure out which structs should actually
 be packed and which should not?  For example, in net_pkt_rcv the
 pointers are not aligned so it will cause a slow down on x86 and crash
 on other arches (which we don't care about).
 
 regards,
 dan carpenter

Dan, 

Thanks for the find. Unfortunately, all the structs defined in iochannel.h
need to be packed since they are shared across different OS and code
instances. I'll look into changing the s-Par firmware and will update the
iochannels structure as well.  

Your reference to x86, does that include x86_64? 

Thanks, 
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 5/5] staging: unisys: add error messages to visornic

[Kershner, David A]

 -Original Message-
 From: Greg KH [mailto:gre...@linuxfoundation.org]
 Sent: Tuesday, June 16, 2015 10:09 PM
 To: Sell, Timothy C
 Cc: driverdev-devel@linuxdriverproject.org; jes.soren...@redhat.com; *S-
 Par-Maintainer; Romer, Benjamin M
 Subject: Re: [PATCH 5/5] staging: unisys: add error messages to visornic
 
 On Wed, Jun 17, 2015 at 12:08:11AM +, Sell, Timothy C wrote:
   -Original Message-
   From: Greg KH [mailto:gre...@linuxfoundation.org]
   Sent: Tuesday, June 16, 2015 5:36 PM
   To: Romer, Benjamin M
   Cc: driverdev-devel@linuxdriverproject.org; jes.soren...@redhat.com;
 *S-
   Par-Maintainer; Sell, Timothy C
   Subject: Re: [PATCH 5/5] staging: unisys: add error messages to visornic
  
   On Mon, Jun 15, 2015 at 11:32:00PM -0400, Benjamin Romer wrote:
From: Tim Sell timothy.s...@unisys.com
   
Add error message to genuine rare error paths, and debug messages
to enable relatively non-verbose tracing of code paths
   
You can enable debug messages by including this on the kernel
 command
   line:
   
visornic.dyndbg=+p
   
or by this from the command line:
   
echo module visornic +p  debugfs/dynamic_debug/control
   
Refer to Documentation/dynamic-debug-howto.txt for more details.
   
In addition to the new debug and error messages, a message like the
following will be logged every time a visornic device is probed, which
will enable you to map back-and-forth between visorbus device names
(e.g., vbus2:dev0) and netdev names (e.g., eth0):
   
visornic vbus2:dev0: visornic_probe success netdev=eth0
   
With this patch and visornic debugging enabled, you should expect to
 see
messages like the following in the most-common scenarios:
   
* driver loaded:
   
  visornic_init
   
* device probed:
   
  visornic vbus2:dev0: visornic_probe
  visor_thread_start
  visor_thread_start success
   
* network interface configured (ifconfig):
   
  net eth0: visornic_open
  net eth0: visornic_enable_with_timeout
  net eth0: visornic_enable_with_timeout success
  net eth0: visornic_open success
   
staging: unisys: More comments from Jes
   
Signed-off-by: Tim Sell timothy.s...@unisys.com
Signed-off-by: David Kershner david.kersh...@unisys.com
Signed-off-by: Benjamin Romer benjamin.ro...@unisys.com
---
 drivers/staging/unisys/visornic/visornic_main.c | 127
   ++--
 1 file changed, 116 insertions(+), 11 deletions(-)
   
diff --git a/drivers/staging/unisys/visornic/visornic_main.c
   b/drivers/staging/unisys/visornic/visornic_main.c
index 7100744..7b9821c 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -294,14 +294,18 @@ static int visor_thread_start(struct
   visor_thread_info *thrinfo,
  int (*threadfn)(void *),
  void *thrcontext, char *name)
 {
+   pr_debug(%s\n, __func__);
  
   These types of entered/exited debugging lines should never be
 needed,
   just use the ftrace infrastructure instead which provides this for you,
   for free, in a much nicer interface.
  
   Please remove these from the patch and resend.
  
   thanks,
  
   greg k-h
 
  Thanks; we will get rid of the offending pr_debug()s from the patch and
  re-send, but please do NOT hold open the window just for this follow-on
  patch.
 
 hold open the window?  What do you mean by this?
 

We want to regroup before we resend this patch to make sure we are
putting the correct messages out. So it will be a few days before the
patch is resent. 

David Kershner

  BTW, I assume it's only the pr_debug()s that you would like
  removed, and that the dev_dbg()s can stay intact (since they also provide
  relevant device name info), right?
 
 I don't know, I didn't get further than this function, sorry.  If they
 are doing nothing more than look at this function this device called!
 then they should be removed.
 
 thanks,
 
 greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 0/3] staging: unisys: visorbus fixes

[Kershner, David A]

Will do. 

Thanks. 

David Kershner

 -Original Message-
 From: Greg KH [mailto:gre...@linuxfoundation.org]
 Sent: Friday, June 12, 2015 2:11 PM
 To: Romer, Benjamin M
 Cc: jes.soren...@redhat.com; driverdev-devel@linuxdriverproject.org; *S-
 Par-Maintainer
 Subject: Re: [PATCH 0/3] staging: unisys: visorbus fixes
 
 On Fri, Jun 12, 2015 at 01:43:31PM -0400, Benjamin Romer wrote:
  This series contains fixes for problems found while testing visorbus.
 
  David Kershner (3):
staging: unisys: Move phys_info to iochannel.h
staging: unisys: convert pack pragma to __packed
staging: unisys: Don't hold device responses until driver loads
 
 Please fix your fixes based on the fixes I proposed for them :)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel