RE: [PATCH] storvsc: Avoid excessive host scan on controller change

[Long Li]

> From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
> Sent: Monday, November 6, 2017 7:40 PM
> To: Long Li 
> Cc: KY Srinivasan ; Haiyang Zhang
> ; Stephen Hemminger
> ; James E . J . Bottomley
> ; Martin K . Petersen
> ; de...@linuxdriverproject.org; linux-
> s...@vger.kernel.org; linux-ker...@vger.kernel.org; Long Li
> 
> Subject: Re: [PATCH] storvsc: Avoid excessive host scan on controller change
> 
> 
> Long,
> 
> > When there are multiple disks attached to the same SCSI controller,
> > the host may send several VSTOR_OPERATION_REMOVE_DEVICE or
> > VSTOR_OPERATION_ENUMERATE_BUS messages in a row, to indicate
> there is
> > a change on the SCSI controller. In response, storvsc rescans the SCSI
> > host.
> 
> Applied to 4.15/scsi-queue with some fuzz. Please verify, thanks!

Martin, thank you! All looking good.

Long

> 
> --
> Martin K. PetersenOracle Linux Engineering
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] storvsc: Avoid excessive host scan on controller change

[Martin K. Petersen]

Long,

> When there are multiple disks attached to the same SCSI controller,
> the host may send several VSTOR_OPERATION_REMOVE_DEVICE or
> VSTOR_OPERATION_ENUMERATE_BUS messages in a row, to indicate there is
> a change on the SCSI controller. In response, storvsc rescans the SCSI
> host.

Applied to 4.15/scsi-queue with some fuzz. Please verify, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 04:56:40PM +0300, Dan Carpenter wrote:
> On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> > > > @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
> > > > *channel, u32 queue,
> > > >   void *msg)
> > > >  {
> > > > int rc;
> > > > -   unsigned long flags;
> > > >  
> > > > if (channel->needs_lock) {
> > > > +   unsigned long flags;
> > > > +
> > > 
> > > Same here, the original code is fine.
> > > 
> > > No idea why the tool wants you to move these around, you should ignore
> > > stuff like that :(
> > 
> > Oh? I'm surprise at this comment. I have always thought limiting scope
> > of local variables was seen as a good thing. Is this a style thing that
> > is on case by case basis or do you never like to declare local variables
> > within blocks?
> > 
> 
> Greg has answered for himself but here are my thoughts...

thanks for taking the time.

> If you look at Colin King's patches, he's using CPPcheck and he's pretty
> religiously moving variables to the localest scope and no one complains.
> It makes sense when he does it from what I have seen.  But mostly he's
> definitely cleaning up the code and fixing bugs and no one cares too
> much about the scope one way or the other.
> 
> But here, I agreed with Greg that the original code was better.  The
> chdr_size and copy_size variables are closely related and are better
> declared together.  The flags declaration was also slightly cleaner at
> the start instead of mixing it up with the code.  Sometimes in a long
> function it definitely makes sense to use a local declaration.  So it's
> a case by case thing.

Cool, got it.

> But mostly no one cares, and I don't want to see hundreds of patches
> mechanically moving declarations around.

Oh bother, scrap that 400 patch series I queued up ... just kidding. 

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

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 04:46:54PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 06, 2017 at 10:59:47AM +0200, Gilad Ben-Yossef wrote:
> > On Mon, Nov 6, 2017 at 10:37 AM, Tobin C. Harding  wrote:
> > > On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> > >> Registers ioread/iowrite operations were done via macros,
> > >> sometime using a "magical" implicit parameter.
> > >>
> > >> Replace all register access with simple inline macros.
> > >>
> > >> Signed-off-by: Gilad Ben-Yossef 
> > >
> > > Hi,
> > >
> > > Nice work. I had a little trouble following this one. Perhaps you are
> > > doing more than one thing per patch, feel free to ignore me if I am
> > > wrong but it seems you are moving the macro definition of CC_REG to a
> > > different header, adding the new inline functions and doing some other
> > > change that I can't grok (commented on below).
> > >
> > > Perhaps this patch could be broken up.
> > 
> > Thank you Tobin.
> > 
> > The original macro that I am replacing had an assumption of a variable void 
> > *
> > cc_base being defined in the context of the macro being called, even though
> > it was not listed in the explicit parameter list of the macro.
> > 
> > The inline function that replace it instead takes an explicit
> > parameter a pointer to
> > struct ssi_drive data * , who has said cc_base as one of the fields.
> > 
> > As a result several function that took a void * cc_base parameter
> > (which is than only
> > used implicitly via the macro without ever being visibly referenced), now 
> > take
> > struct ssi_drive data * parameter instead which is passed explicitly
> > to the inline
> > function.
> > 
> > These seems to be the places you are referring to. They are cascading 
> > changes
> > resulting from the change in API between the macro and the inline
> > function that replaces it.
> > 
> > I imagine I can try to break that change to two patches but at least
> > in my mind this is artificial
> > and it is a single logical change.
> > 
> > Having said that, if you think otherwise and consider this
> > none-reviewable even after this
> > explanation let me know and  I'd be happy to break it down.
> 
> Nah, this is fine, I'll take it as-is.  Tobin, thanks for the review.

No worries. Greg make sure you yell at me if I start causing you more
work than I'm saving. It's a fine line reviewing patches when you are
not super experienced.

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

Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging

[Roy Pledge]

>> struct dpaa2_io {
>>   atomic_t refs;
>>
>> That's a kref, please use it instead of trying to roll your own.
>>
>> And even for this, your locking is not correct (i.e. you do not have
>> any), that needs to be fixed so that teardown works correctly.
> 
> I think we can drop this refcount altogether as it's not used. Roy, any
> comment on this?
> 

Yes I think this refcount can be removed.  I'll make a note for when the 
DPIO is moving out of staging but that isn't part of this patchset. 
There are other cleanups needed in DPIO as well. I've been holding off 
on pushing patches for that until the bus driver gets moved to try to 
avoid complex patch dependencies and merge conflict confusion.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] storvsc: Avoid excessive host scan on controller change

[Cathy Avery]

On 10/31/2017 05:58 PM, Long Li wrote:

From: Long Li 

When there are multiple disks attached to the same SCSI controller,
the host may send several VSTOR_OPERATION_REMOVE_DEVICE or
VSTOR_OPERATION_ENUMERATE_BUS messages in a row, to indicate there is a
change on the SCSI controller. In response, storvsc rescans the SCSI host.

There is no need to do multiple scans on the same host. Fix the code to do
only one scan.

Signed-off-by: Long Li 
---
  drivers/scsi/storvsc_drv.c | 26 +++---
  1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 6febcdb..b602f52 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -488,6 +488,8 @@ struct hv_host_device {
unsigned char target;
struct workqueue_struct *handle_error_wq;
char work_q_name[20];
+   struct work_struct host_scan_work;
+   struct Scsi_Host *host;
  };
  
  struct storvsc_scan_work {

@@ -516,13 +518,12 @@ static void storvsc_device_scan(struct work_struct *work)
  
  static void storvsc_host_scan(struct work_struct *work)

  {
-   struct storvsc_scan_work *wrk;
struct Scsi_Host *host;
struct scsi_device *sdev;
+   struct hv_host_device *host_device =
+   container_of(work, struct hv_host_device, host_scan_work);
  
-	wrk = container_of(work, struct storvsc_scan_work, work);

-   host = wrk->host;
-
+   host = host_device->host;
/*
 * Before scanning the host, first check to see if any of the
 * currrently known devices have been hot removed. We issue a
@@ -542,8 +543,6 @@ static void storvsc_host_scan(struct work_struct *work)
 * Now scan the host to discover LUNs that may have been added.
 */
scsi_scan_host(host);
-
-   kfree(wrk);
  }
  
  static void storvsc_remove_lun(struct work_struct *work)

@@ -1119,8 +1118,7 @@ static void storvsc_on_receive(struct storvsc_device 
*stor_device,
 struct vstor_packet *vstor_packet,
 struct storvsc_cmd_request *request)
  {
-   struct storvsc_scan_work *work;
-
+   struct hv_host_device *host_dev;
switch (vstor_packet->operation) {
case VSTOR_OPERATION_COMPLETE_IO:
storvsc_on_io_completion(stor_device, vstor_packet, request);
@@ -1128,13 +1126,9 @@ static void storvsc_on_receive(struct storvsc_device 
*stor_device,
  
  	case VSTOR_OPERATION_REMOVE_DEVICE:

case VSTOR_OPERATION_ENUMERATE_BUS:
-   work = kmalloc(sizeof(struct storvsc_scan_work), GFP_ATOMIC);
-   if (!work)
-   return;
-
-   INIT_WORK(>work, storvsc_host_scan);
-   work->host = stor_device->host;
-   schedule_work(>work);
+   host_dev = shost_priv(stor_device->host);
+   queue_work(
+   host_dev->handle_error_wq, _dev->host_scan_work);
break;
  
  	case VSTOR_OPERATION_FCHBA_DATA:

@@ -1747,6 +1741,7 @@ static int storvsc_probe(struct hv_device *device,
  
  	host_dev->port = host->host_no;

host_dev->dev = device;
+   host_dev->host = host;
  
  
  	stor_device = kzalloc(sizeof(struct storvsc_device), GFP_KERNEL);

@@ -1815,6 +1810,7 @@ static int storvsc_probe(struct hv_device *device,
create_singlethread_workqueue(host_dev->work_q_name);
if (!host_dev->handle_error_wq)
goto err_out2;
+   INIT_WORK(_dev->host_scan_work, storvsc_host_scan);
/* Register the HBA and start the scsi bus scan */
ret = scsi_add_host(host, >device);
if (ret != 0)


I've tested this patch with both a multipath failover while running fio 
and taking hyperV snap shots while luns are being hot added and removed.


Tested-by: Cathy Avery 



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

[PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Signed-off-by: Benjamin Gaignard 
---
 drivers/staging/android/TODO|  1 -
 drivers/staging/android/ion/Kconfig |  7 
 drivers/staging/android/ion/ion-ioctl.c | 18 --
 drivers/staging/android/ion/ion.c   | 62 +
 drivers/staging/android/ion/ion.h   | 15 ++--
 5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -8,7 +8,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman  and Cc:
diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
 
+config ION_LEGACY_DEVICE_API
+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ union ion_ioctl_arg {
struct ion_heap_query query;
 };
 
-static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+static int validate_ioctl_arg(struct file *filp,
+ unsigned int cmd, union ion_ioctl_arg *arg)
 {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
 
-   ret = validate_ioctl_arg(cmd, );
+   ret = validate_ioctl_arg(filp, cmd, );
if (WARN_ON_ONCE(ret))
return ret;
 
diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,9 @@
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+#define ION_NAME "ion"
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+static struct device ion_bus = {
+   .init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+   .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
 {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
 
if (!heap->ops->allocate || !heap->ops->free)
pr_err("%s: can not add heap with invalid ops struct.\n",
   __func__);
 
+   if (heap_id >= ION_DEV_MAX)
+   return -EBUSY;
+
+   heap->ddev.parent = _bus;
+   heap->ddev.bus = _bus_type;
+   heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id);
+   dev_set_name(>ddev, ION_NAME"%d", heap_id);
+   device_initialize(>ddev);
+   cdev_init(>chrdev, _fops);
+   heap->chrdev.owner = THIS_MODULE;
+   ret = cdev_device_add(>chrdev, >ddev);
+   if (ret < 0)
+   return ret;
+

[PATCH v6 0/2] staging: ion: get one device per heap

[Benjamin Gaignard]

version 6:
- add an ION bus so heap are show as devices in /sys/bus/ion/
  instead of platform bus.
- split the patch in two: one for include reordering and one
  for per heap device change
- rebased on top of next-2017110 tag

version 5:
- create a configuration flag to keep legacy Ion misc device

version 4:
- add a configuration flag to switch between legacy Ion misc device
  and one device per heap version.
  This change has been suggested after Laura talks at XDC 2017.

version 3:
- change ion_device_add_heap prototype to return a possible error.

version 2:
- simplify ioctl check like propose by Dan
- make sure that we don't register more than ION_DEV_MAX heaps.

Until now all ion heaps are addressing using the same device "/dev/ion".
This way of working doesn't allow to give access rights (for example with
SElinux rules) per heap.
This series propose to have one device "/dev/ionX" per heap.
Query heaps informations will be possible on each device node but
allocation request will only be possible if heap_mask_id match with device 
minor number.

Using legacy Ion misc device is still by setting ION_LEGACY_DEVICE_API
configuration flag.


Benjamin Gaignard (2):
  staging: ion: reorder include
  staging: ion: create one device entry per heap

 drivers/staging/android/TODO|  1 -
 drivers/staging/android/ion/Kconfig |  7 +++
 drivers/staging/android/ion/ion-ioctl.c | 18 +++-
 drivers/staging/android/ion/ion.c   | 76 +++--
 drivers/staging/android/ion/ion.h   | 15 ++-
 5 files changed, 98 insertions(+), 19 deletions(-)

-- 
2.7.4

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

[PATCH v6 1/2] staging: ion: reorder include

[Benjamin Gaignard]

Put include in alphabetic order

Signed-off-by: Benjamin Gaignard 
---
 drivers/staging/android/ion/ion.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index a7d9b0e..fda9756 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -15,28 +15,28 @@
  *
  */
 
+#include 
+#include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
+#include 
 #include 
 #include 
-#include 
-#include 
-#include 
-#include 
 
 #include "ion.h"
 
-- 
2.7.4

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

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Greg Kroah-Hartman]

On Mon, Nov 06, 2017 at 10:59:47AM +0200, Gilad Ben-Yossef wrote:
> On Mon, Nov 6, 2017 at 10:37 AM, Tobin C. Harding  wrote:
> > On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> >> Registers ioread/iowrite operations were done via macros,
> >> sometime using a "magical" implicit parameter.
> >>
> >> Replace all register access with simple inline macros.
> >>
> >> Signed-off-by: Gilad Ben-Yossef 
> >
> > Hi,
> >
> > Nice work. I had a little trouble following this one. Perhaps you are
> > doing more than one thing per patch, feel free to ignore me if I am
> > wrong but it seems you are moving the macro definition of CC_REG to a
> > different header, adding the new inline functions and doing some other
> > change that I can't grok (commented on below).
> >
> > Perhaps this patch could be broken up.
> 
> Thank you Tobin.
> 
> The original macro that I am replacing had an assumption of a variable void *
> cc_base being defined in the context of the macro being called, even though
> it was not listed in the explicit parameter list of the macro.
> 
> The inline function that replace it instead takes an explicit
> parameter a pointer to
> struct ssi_drive data * , who has said cc_base as one of the fields.
> 
> As a result several function that took a void * cc_base parameter
> (which is than only
> used implicitly via the macro without ever being visibly referenced), now take
> struct ssi_drive data * parameter instead which is passed explicitly
> to the inline
> function.
> 
> These seems to be the places you are referring to. They are cascading changes
> resulting from the change in API between the macro and the inline
> function that replaces it.
> 
> I imagine I can try to break that change to two patches but at least
> in my mind this is artificial
> and it is a single logical change.
> 
> Having said that, if you think otherwise and consider this
> none-reviewable even after this
> explanation let me know and  I'd be happy to break it down.

Nah, this is fine, I'll take it as-is.  Tobin, thanks for the review.

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

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Mon, Nov 06, 2017 at 03:42:04PM +0100, Benjamin Gaignard wrote:
> 2017-11-02 12:10 GMT+01:00 Mark Brown :
> > On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
> >> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
> >
> >> > There was a discussion a while ago in the context of I2C/SPI MFDs
> >> > which concluded that if you need a bus and it's going to be effectively
> >> > noop then you should just use the platform bus as anything else will
> >> > consist almost entirely of cut'n'paste from the platform bus with some
> >> > light sed usage and code duplication is bad.  It's not super lovely as
> >> > it's not actually a memory mapped device but it's the best idea we've
> >> > got.
> >
> >> Ugh, I hate that.  What's wrong with using a "virtual" device instead?
> >
> > It was the duplication, initially everyone was making buses.
> >
> >> I can create a "virtual" bus for things like this if they really want a
> >> "simple" bus, abusing platform for this is the major reason I hate the
> >> platform bus code...
> >
> > In the MFD case they're physical devices, they're just usually on the
> > wrong side of an I2C or SPI link.  Plus MFD already handles platform
> > devices for things that are memory mapped so it's a bit of a more
> > natural fit there.
> 
> What I can do is to register an ion bus (like cec one for example),
> add one ion parent device so heaps will appear in /sys/bus/ion/ion*
> and /sys/devices/ion/ion*
> 
> Does that could sound good enough ?

I would like to see that...

thanks,

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

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-11-02 12:10 GMT+01:00 Mark Brown :
> On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
>> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
>
>> > There was a discussion a while ago in the context of I2C/SPI MFDs
>> > which concluded that if you need a bus and it's going to be effectively
>> > noop then you should just use the platform bus as anything else will
>> > consist almost entirely of cut'n'paste from the platform bus with some
>> > light sed usage and code duplication is bad.  It's not super lovely as
>> > it's not actually a memory mapped device but it's the best idea we've
>> > got.
>
>> Ugh, I hate that.  What's wrong with using a "virtual" device instead?
>
> It was the duplication, initially everyone was making buses.
>
>> I can create a "virtual" bus for things like this if they really want a
>> "simple" bus, abusing platform for this is the major reason I hate the
>> platform bus code...
>
> In the MFD case they're physical devices, they're just usually on the
> wrong side of an I2C or SPI link.  Plus MFD already handles platform
> devices for things that are memory mapped so it's a bit of a more
> natural fit there.

What I can do is to register an ion bus (like cec one for example),
add one ion parent device so heaps will appear in /sys/bus/ion/ion*
and /sys/devices/ion/ion*

Does that could sound good enough ?

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

Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging

[Laurentiu Tudor]

On 11/03/2017 05:17 PM, Greg KH wrote:
> On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote:
>> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote:
>>> From: Stuart Yoder 
>>>
>>> Move the source files out of staging into their final locations:
>>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl
>>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
>>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
>>>-README.txt, providing and overview of DPAA goes to
>>> Documentation/dpaa2/overview.txt
>>>
>>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO.
>>> Update dpaa2_eth and dpio staging drivers.
>>>
>>> Signed-off-by: Stuart Yoder 
>>> Signed-off-by: Laurentiu Tudor 
>>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates]
>>> Cc: Thomas Gleixner 
>>> Cc: Jason Cooper 
>>> Cc: Marc Zyngier 
>>
>> This is going to have to wait until I get a chunk of time to do the
>> review.  Probably after 4.13-final is out.
>
> Ok, review comments as I go through the code:
>   mc-sys.c 323 EXPORT_SYMBOL(mc_send_command);
>
> should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your
> other exports and global namespace, right?

Right. There's an inconsistent mixture of EXPORT_SYMBOL() and 
EXPORT_SYMBOL_GPL() usage. I'll change them all to EXPORT_SYMBOL_GPL().

> You export a lot of dpcon_* symbols that no one uses, please do not do
> that.  Verify that all of them are actually used right now, if not,
> remove them.  If you think you are going to use them in the future,
> wonderful, add them in then.

Actually, most of the dpcon_* APIs are used in the ethernet driver here: 
drivers/staging/fsl-dpaa2/ethernet. I think i saw only a couple of 
functions that are not used so I'll drop those.

> Same for your dpaa2_* exported symbols, most are not used from what I
> can see.

I'll check these too and drop the unused ones.

> struct dpaa2_io {
>  atomic_t refs;
>
> That's a kref, please use it instead of trying to roll your own.
>
> And even for this, your locking is not correct (i.e. you do not have
> any), that needs to be fixed so that teardown works correctly.

I think we can drop this refcount altogether as it's not used. Roy, any 
comment on this?

> You have a lot of WARN_ON() calls, that's going to be ignored and should
> all not be needed now that the code is debugged and working properly.
> Please remove them, or turn them into dev_err() calls with a real if ()
> check instead.

Right, there are quite of few (100+) WARN_ON()s. I'll go through them 
and clean them up ... most of them seem paranoid checks anyway.

> You are checking "strings" for the type of device in a lot of places,
> like this:
>   if (strcmp(obj_desc->type, "dprc") == 0) {
> why are you not just using the built-in driver model .type field and
> comparing that to the different type structures?  It's much easier, and
> you don't have to again, "roll your own".  See the USB or Greybus code
> for examples of busses that have different "types" of devices on them at
> the same time.
>

I had a quick look over greybus and noticed the device types declared in 
drivers/staging/greybus/greybus.h plus some helper macros to check the 
device type. I'll give this a go, though i might return with some 
questions (e.g. I don't yet understand what device_type::release op is 
used for).

---
Thanks & Best Regards, Laurentiu
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[Dan Carpenter]

On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> > > @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
> > > *channel, u32 queue,
> > > void *msg)
> > >  {
> > >   int rc;
> > > - unsigned long flags;
> > >  
> > >   if (channel->needs_lock) {
> > > + unsigned long flags;
> > > +
> > 
> > Same here, the original code is fine.
> > 
> > No idea why the tool wants you to move these around, you should ignore
> > stuff like that :(
> 
> Oh? I'm surprise at this comment. I have always thought limiting scope
> of local variables was seen as a good thing. Is this a style thing that
> is on case by case basis or do you never like to declare local variables
> within blocks?
> 

Greg has answered for himself but here are my thoughts...

If you look at Colin King's patches, he's using CPPcheck and he's pretty
religiously moving variables to the localest scope and no one complains.
It makes sense when he does it from what I have seen.  But mostly he's
definitely cleaning up the code and fixing bugs and no one cares too
much about the scope one way or the other.

But here, I agreed with Greg that the original code was better.  The
chdr_size and copy_size variables are closely related and are better
declared together.  The flags declaration was also slightly cleaner at
the start instead of mixing it up with the code.  Sometimes in a long
function it definitely makes sense to use a local declaration.  So it's
a case by case thing.

But mostly no one cares, and I don't want to see hundreds of patches
mechanically moving declarations around.

regards,
dan carpenter

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

Re: [RFC 05/12] dt-bindings: mtd: add Marvell NAND controller documentation

[Miquel RAYNAL]

Hi Rob,

> > +Required properties:
> > +C'est faux, t'en a rajouté un y a pas longtps :).
> > +Je conseille de mettre ça sous forme de liste, genre  
> 
> Humm.

Oops!

> 
> > +
> > +- compatible: can be one of the following:
> > +* "marvell,armada-8k-nand-controller"
> > +* "marvell,armada370-nand-controller"
> > +* "marvell,pxa3xx-nand-controller"
> > +* "marvell,armada-8k-nand" (deprecated)
> > +* "marvell,armada370-nand" (deprecated)
> > +* "marvell,pxa3xx-nand" (deprecated)
> > +- reg: shall contain registers location and length for data and
> > reg.  
> 
> 2 regions?

Just one, rephrased.

> 
> > +- #address-cells: shall be set to 1. Encode the nand CS.
> > +- #size-cells: shall be set to 0.
> > +- interrupts: shall define the nand controller interrupt.
> > +- clocks: shall reference nand controller clocks.  
> 
> How many clocks?

Only one too: "reference the NAND controller clock".

> 
> > +- marvell,system-controller: Set to retrieve the syscon node that
> > handles
> > +  NAND controller related registers (only required with the
> > +  "marvell,armada-8k-nand[-controller]" compatibles).
> > +
> > +Optional properties:
> > +- dmas: shall reference DMA channel associated to the NAND
> > controller. +- dma-names: shall be "rxtx".
> > +
> > +Optional children nodes:
> > +Children nodes represent the available NAND chips.
> > +
> > +Required properties:
> > +- reg: shall contain the native Chip Select ids (0-3)
> > +- marvell,rb: shall contain the native Ready/Busy ids (0-1)
> > +
> > +Optional properties:
> > +- marvell,nand-keep-config: orders the driver not to take the
> > timings
> > +  from the core and leaving them completely untouched. Bootloader
> > +  timings will then be used.
> > +- marvell,nand-enable-arbiter: only useful for PXA platforms, will
> > +  enable bus arbiter between NFC and DFI bus (must be enabled for
> > +  NFC operation)  
> 
> Why do you need this if it must be enabled?

That is right, there is no more need for it, also removed it from the
driver, just knowing the board with the compatible string is enough.

> 
> > +- nand-on-flash-bbt: speed up the boot process by not discovering
> > all
> > +  the bad blocks at each boot and reading directly an on flash
> > table. +- nand-ecc-mode: one of the supported ECC modes ("none",
> > "soft",
> > +  "hw"). If not specified, hardware ECC will be used.
> > +- nand-ecc-algo: algorithm to use if previous choice was "soft"
> > +  ("hamming" or "bch). This property may be added for hardware ECC
> > for
> > +  clarification but will be ignored by the driver because ECC mode
> > is
> > +  chosen depending on the page size and the strength required by
> > the
> > +  NAND chip. This value may be overwritten with the
> > nand-ecc-strength
> > +  property.
> > +- nand-ecc-strength: desired ECC strength.
> > +- nand-ecc-step-size: indication on the ECC step size. This has no
> > +  effect and will be ignored by the driver when using hardware
> > +  ECC. Because Marvell's NAND flash controller does use fixed
> > strength
> > +  (1-bit for Hamming, 16-bit for BCH), the step size will shrink or
> > +  grown in order to fit the required strength and the value
> > +  updated. Step sizes are not completely random for all and follow
> > +  certain patterns described in AN-379, "Marvell SoC NFC ECC".  
> 
> For standard properties, just reference nand.txt and add any 
> constraints. Don't define what the property is again.

Ok.

> 
> > +
> > +See Documentation/devicetree/bindings/mtd/nand.txt for more
> > details on +generic bindings.
> > +
> > +
> > +Example:
> > +nand_controller: nand-controller@d {
> > +   compatible = "marvell,armada370-nand-controller";
> > +   reg = <0xd 0x54>;
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +   interrupts = ;
> > +   clocks = < 0>;
> > +   status = "okay";  
> 
> Don't show status in examples.

Ok.

> 
> > +
> > +   nand@0 {
> > +   reg = <0>;
> > +   marvell,rb = <0>;
> > +   nand-ecc-mode = "hw";
> > +   marvell,nand-keep-config;
> > +   marvell,nand-enable-arbiter;
> > +   nand-on-flash-bbt;
> > +   nand-ecc-strength = <4>;
> > +   nand-ecc-step-size = <512>;
> > +
> > +   partitions {
> > +   compatible = "fixed-partitions";
> > +   #address-cells = <1>;
> > +   #size-cells = <1>;
> > +
> > +   partition@0 {
> > +   label = "Rootfs";
> > +   reg = <0x 0x4000>;
> > +   };
> > +   };
> > +   };
> > +};
> > -- 
> > 2.11.0
> >   

Thanks for the review,
Miquèl
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 0/9] Hyper-V: paravirtualized remote TLB flushing and hypercall improvements

[Wanpeng Li]

2017-11-06 18:10 GMT+08:00 Vitaly Kuznetsov :
> Wanpeng Li  writes:
>
>> 2017-11-06 17:14 GMT+08:00 Vitaly Kuznetsov :
>>> Wanpeng Li  writes:
>>>
 2017-08-03 0:09 GMT+08:00 Vitaly Kuznetsov :
> Changes since v9:
> - Rebase to 4.13-rc3.
> - Drop PATCH1 as it was already taken by Greg to char-misc tree. There're 
> no
>   functional dependencies on this patch so the series can go through a 
> different tree
>   (and it actually belongs to x86 if I got Ingo's comment right).
> - Add in missing void return type in PATCH1 [Colin King, Ingo Molnar, 
> Greg KH]
> - A few minor fixes in what is now PATCH7: add pr_fmt, tiny style fix in
>   hyperv_flush_tlb_others() [Andy Shevchenko]
> - Fix "error: implicit declaration of function 'virt_to_phys'" in PATCH2
>   reported by kbuild test robot (#include )
> - Add Steven's 'Reviewed-by:' to PATCH9.
>
> Original description:
>
> Hyper-V supports hypercalls for doing local and remote TLB flushing and
> gives its guests hints when using hypercall is preferred. While doing
> hypercalls for local TLB flushes is probably not practical (and is not
> being suggested by modern Hyper-V versions) remote TLB flush with a
> hypercall brings significant improvement.
>
> To test the series I wrote a special 'TLB trasher': on a 16 vCPU guest I
> was creating 32 threads which were doing 10 mmap/munmaps each on some
> big file. Here are the results:
>
> Before:
> # time ./pthread_mmap ./randfile
> real3m33.118s
> user0m3.698s
> sys 3m16.624s
>
> After:
> # time ./pthread_mmap ./randfile
> real2m19.920s
> user0m2.662s
> sys 2m9.948s
>
> This series brings a number of small improvements along the way: fast
> hypercall implementation and using it for event signaling, rep hypercalls
> implementation, hyperv tracing subsystem (which only traces the newly 
> added
> remote TLB flush for now).
>

 Hi Vitaly,

 Could you attach your benchmark? I'm interested in to try the
 implementation in paravirt kvm.

>>>
>>> Oh, this would be cool) I briefly discussed the idea with Radim (one of
>>> KVM maintainers) during the last KVM Forum and he wasn't opposed to the
>>> idea. Need to talk to Paolo too. Good thing is that we have everything
>>
>> I talk with Paolo today and he points this feature to me, so I believe
>> he likes it. :) In addition,
>> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
>> I search Hypervisor Top Level Functional Specification v5.0b.pdf
>> document but didn't find a section introduce the Hyper-V:
>> paravirtualized remote TLB flushing and hypercall stuff, could you
>> point out?
>>
>
> It's there, search for
> HvFlushVirtualAddressSpace/HvFlushVirtualAddressSpaceEx and
> HvFlushVirtualAddressList/HvFlushVirtualAddressListEx.

Got it, thanks.

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

Re: [PATCH v10 0/9] Hyper-V: paravirtualized remote TLB flushing and hypercall improvements

[Vitaly Kuznetsov]

Wanpeng Li  writes:

> 2017-11-06 17:14 GMT+08:00 Vitaly Kuznetsov :
>> Wanpeng Li  writes:
>>
>>> 2017-08-03 0:09 GMT+08:00 Vitaly Kuznetsov :
 Changes since v9:
 - Rebase to 4.13-rc3.
 - Drop PATCH1 as it was already taken by Greg to char-misc tree. There're 
 no
   functional dependencies on this patch so the series can go through a 
 different tree
   (and it actually belongs to x86 if I got Ingo's comment right).
 - Add in missing void return type in PATCH1 [Colin King, Ingo Molnar, Greg 
 KH]
 - A few minor fixes in what is now PATCH7: add pr_fmt, tiny style fix in
   hyperv_flush_tlb_others() [Andy Shevchenko]
 - Fix "error: implicit declaration of function 'virt_to_phys'" in PATCH2
   reported by kbuild test robot (#include )
 - Add Steven's 'Reviewed-by:' to PATCH9.

 Original description:

 Hyper-V supports hypercalls for doing local and remote TLB flushing and
 gives its guests hints when using hypercall is preferred. While doing
 hypercalls for local TLB flushes is probably not practical (and is not
 being suggested by modern Hyper-V versions) remote TLB flush with a
 hypercall brings significant improvement.

 To test the series I wrote a special 'TLB trasher': on a 16 vCPU guest I
 was creating 32 threads which were doing 10 mmap/munmaps each on some
 big file. Here are the results:

 Before:
 # time ./pthread_mmap ./randfile
 real3m33.118s
 user0m3.698s
 sys 3m16.624s

 After:
 # time ./pthread_mmap ./randfile
 real2m19.920s
 user0m2.662s
 sys 2m9.948s

 This series brings a number of small improvements along the way: fast
 hypercall implementation and using it for event signaling, rep hypercalls
 implementation, hyperv tracing subsystem (which only traces the newly added
 remote TLB flush for now).

>>>
>>> Hi Vitaly,
>>>
>>> Could you attach your benchmark? I'm interested in to try the
>>> implementation in paravirt kvm.
>>>
>>
>> Oh, this would be cool) I briefly discussed the idea with Radim (one of
>> KVM maintainers) during the last KVM Forum and he wasn't opposed to the
>> idea. Need to talk to Paolo too. Good thing is that we have everything
>
> I talk with Paolo today and he points this feature to me, so I believe
> he likes it. :) In addition,
> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> I search Hypervisor Top Level Functional Specification v5.0b.pdf
> document but didn't find a section introduce the Hyper-V:
> paravirtualized remote TLB flushing and hypercall stuff, could you
> point out?
>

It's there, search for
HvFlushVirtualAddressSpace/HvFlushVirtualAddressSpaceEx and
HvFlushVirtualAddressList/HvFlushVirtualAddressListEx.

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

Re: [PATCH v10 0/9] Hyper-V: paravirtualized remote TLB flushing and hypercall improvements

[Wanpeng Li]

2017-11-06 17:14 GMT+08:00 Vitaly Kuznetsov :
> Wanpeng Li  writes:
>
>> 2017-08-03 0:09 GMT+08:00 Vitaly Kuznetsov :
>>> Changes since v9:
>>> - Rebase to 4.13-rc3.
>>> - Drop PATCH1 as it was already taken by Greg to char-misc tree. There're no
>>>   functional dependencies on this patch so the series can go through a 
>>> different tree
>>>   (and it actually belongs to x86 if I got Ingo's comment right).
>>> - Add in missing void return type in PATCH1 [Colin King, Ingo Molnar, Greg 
>>> KH]
>>> - A few minor fixes in what is now PATCH7: add pr_fmt, tiny style fix in
>>>   hyperv_flush_tlb_others() [Andy Shevchenko]
>>> - Fix "error: implicit declaration of function 'virt_to_phys'" in PATCH2
>>>   reported by kbuild test robot (#include )
>>> - Add Steven's 'Reviewed-by:' to PATCH9.
>>>
>>> Original description:
>>>
>>> Hyper-V supports hypercalls for doing local and remote TLB flushing and
>>> gives its guests hints when using hypercall is preferred. While doing
>>> hypercalls for local TLB flushes is probably not practical (and is not
>>> being suggested by modern Hyper-V versions) remote TLB flush with a
>>> hypercall brings significant improvement.
>>>
>>> To test the series I wrote a special 'TLB trasher': on a 16 vCPU guest I
>>> was creating 32 threads which were doing 10 mmap/munmaps each on some
>>> big file. Here are the results:
>>>
>>> Before:
>>> # time ./pthread_mmap ./randfile
>>> real3m33.118s
>>> user0m3.698s
>>> sys 3m16.624s
>>>
>>> After:
>>> # time ./pthread_mmap ./randfile
>>> real2m19.920s
>>> user0m2.662s
>>> sys 2m9.948s
>>>
>>> This series brings a number of small improvements along the way: fast
>>> hypercall implementation and using it for event signaling, rep hypercalls
>>> implementation, hyperv tracing subsystem (which only traces the newly added
>>> remote TLB flush for now).
>>>
>>
>> Hi Vitaly,
>>
>> Could you attach your benchmark? I'm interested in to try the
>> implementation in paravirt kvm.
>>
>
> Oh, this would be cool) I briefly discussed the idea with Radim (one of
> KVM maintainers) during the last KVM Forum and he wasn't opposed to the
> idea. Need to talk to Paolo too. Good thing is that we have everything

I talk with Paolo today and he points this feature to me, so I believe
he likes it. :) In addition,
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
I search Hypervisor Top Level Functional Specification v5.0b.pdf
document but didn't find a section introduce the Hyper-V:
paravirtualized remote TLB flushing and hypercall stuff, could you
point out?

Regards,
Wanpeng Li

> in place for guests now (HAVE_RCU_TABLE_FREE is enabled globaly on x86).
>
> Please see the microbenchmark attached. Adjust defines in the beginning
> to match your needs. It is not anything smart, basically just a TLB
> trasher.
>
> In theory, the best result is achived when we're overcommiting the host
> by running multiple vCPUs on each pCPU. In this case PV tlb flush avoids
> touching vCPUs which are not scheduled and avoid the wait on the main
> CPU.
>
> --
>   Vitaly
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 4/4] staging: greybus: loopback: convert loopback to use generic async operations

[Johan Hovold]

On Mon, Nov 06, 2017 at 01:32:22AM +, Bryan O'Donoghue wrote:
> Loopback has its own internal method for tracking and timing out
> asynchronous operations however previous patches make it possible to use
> functionality provided by operation.c to do this instead. Using the code in
> operation.c means we can completely subtract the timer, the work-queue, the
> kref and the cringe-worthy 'pending' flag. The completion callback
> triggered by operation.c will provide an authoritative result code -
> including -ETIMEDOUT for asynchronous operations.
> 
> Signed-off-by: Bryan O'Donoghue 

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

Re: [PATCH v3 2/4] staging: greybus: loopback: Fix iteration count on async path

[Johan Hovold]

On Mon, Nov 06, 2017 at 01:32:20AM +, Bryan O'Donoghue wrote:
> Commit 12927835d211 ("greybus: loopback: Add asynchronous bi-directional
> support") does what it says on the tin - namely, adds support for
> asynchronous bi-directional loopback operations.
> 
> What it neglects to do though is increment the per-connection
> gb->iteration_count on an asynchronous operation error. This patch fixes
> that omission.
> 
> Fixes: 12927835d211 ("greybus: loopback: Add asynchronous bi-directional 
> support")
> 
> Signed-off-by: Bryan O'Donoghue 
> Reported-by: Mitch Tasman 

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

Re: [PATCH v3 1/4] staging: greybus: loopback: Hold per-connection mutex across operations

[Johan Hovold]

On Mon, Nov 06, 2017 at 01:32:19AM +, Bryan O'Donoghue wrote:
> Commit d9fb3754ecf8 ("greybus: loopback: Relax locking during loopback
> operations") changes the holding of the per-connection mutex to be less
> restrictive because at the time of that commit per-connection mutexes were
> encapsulated by a per-driver level gb_dev.mutex.
> 
> Commit 8e1d6c336d74 ("greybus: loopback: drop bus aggregate calculation")
> on the other hand subtracts the driver level gb_dev.mutex but neglects to
> move the mutex back to the place it was prior to commit d9fb3754ecf8
> ("greybus: loopback: Relax locking during loopback operations"), as a
> result several members of the per connection struct gb_loopback are racy.
> 
> The solution is restoring the old location of mutex_unlock(>mutex) as
> it was in commit d9fb3754ecf8 ("greybus: loopback: Relax locking during
> loopback operations").
> 
> Fixes: 8e1d6c336d74 ("greybus: loopback: drop bus aggregate calculation")
> 
> Signed-off-by: Bryan O'Donoghue 
> Cc: Johan Hovold 
> Cc: Alex Elder 
> Cc: Greg Kroah-Hartman 
> Cc: Mitch Tasman 
> Cc: greybus-...@lists.linaro.org
> Cc: de...@driverdev.osuosl.org
> Cc: linux-ker...@vger.kernel.org
> ---

This looks like it addresses both my comments on v1 (mutex and error
stats). Thanks for tracking it down.

Reviewed-by: Johan Hovold 

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

Re: [PATCH v10 0/9] Hyper-V: paravirtualized remote TLB flushing and hypercall improvements

[Vitaly Kuznetsov]

Wanpeng Li  writes:

> 2017-08-03 0:09 GMT+08:00 Vitaly Kuznetsov :
>> Changes since v9:
>> - Rebase to 4.13-rc3.
>> - Drop PATCH1 as it was already taken by Greg to char-misc tree. There're no
>>   functional dependencies on this patch so the series can go through a 
>> different tree
>>   (and it actually belongs to x86 if I got Ingo's comment right).
>> - Add in missing void return type in PATCH1 [Colin King, Ingo Molnar, Greg 
>> KH]
>> - A few minor fixes in what is now PATCH7: add pr_fmt, tiny style fix in
>>   hyperv_flush_tlb_others() [Andy Shevchenko]
>> - Fix "error: implicit declaration of function 'virt_to_phys'" in PATCH2
>>   reported by kbuild test robot (#include )
>> - Add Steven's 'Reviewed-by:' to PATCH9.
>>
>> Original description:
>>
>> Hyper-V supports hypercalls for doing local and remote TLB flushing and
>> gives its guests hints when using hypercall is preferred. While doing
>> hypercalls for local TLB flushes is probably not practical (and is not
>> being suggested by modern Hyper-V versions) remote TLB flush with a
>> hypercall brings significant improvement.
>>
>> To test the series I wrote a special 'TLB trasher': on a 16 vCPU guest I
>> was creating 32 threads which were doing 10 mmap/munmaps each on some
>> big file. Here are the results:
>>
>> Before:
>> # time ./pthread_mmap ./randfile
>> real3m33.118s
>> user0m3.698s
>> sys 3m16.624s
>>
>> After:
>> # time ./pthread_mmap ./randfile
>> real2m19.920s
>> user0m2.662s
>> sys 2m9.948s
>>
>> This series brings a number of small improvements along the way: fast
>> hypercall implementation and using it for event signaling, rep hypercalls
>> implementation, hyperv tracing subsystem (which only traces the newly added
>> remote TLB flush for now).
>>
>
> Hi Vitaly,
>
> Could you attach your benchmark? I'm interested in to try the
> implementation in paravirt kvm.
>

Oh, this would be cool) I briefly discussed the idea with Radim (one of
KVM maintainers) during the last KVM Forum and he wasn't opposed to the
idea. Need to talk to Paolo too. Good thing is that we have everything
in place for guests now (HAVE_RCU_TABLE_FREE is enabled globaly on x86).

Please see the microbenchmark attached. Adjust defines in the beginning
to match your needs. It is not anything smart, basically just a TLB
trasher.

In theory, the best result is achived when we're overcommiting the host
by running multiple vCPUs on each pCPU. In this case PV tlb flush avoids
touching vCPUs which are not scheduled and avoid the wait on the main
CPU.

-- 
  Vitaly

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define nthreads 48
#define pagecount 16384
#define nrounds 1000
#define nchunks 20
#define PAGE_SIZE 4096

int fd;
unsigned long v;

void *threadf(void *ptr)
{
unsigned long *addr[nchunks];
int i, j, k;
struct timespec ts = {0};
int ret;

ts.tv_nsec = random() % 1024;

for (j = 0; j < nrounds; j++) {
for (i = 0; i < nchunks; i++) {
addr[i] = mmap(NULL, PAGE_SIZE * pagecount, PROT_READ, 
MAP_SHARED, fd, i * PAGE_SIZE);
if (addr[i] == MAP_FAILED) {
fprintf(stderr, "mmap\n");
exit(1);
}
}

nanosleep(, NULL);

for (i = 0; i < nchunks; i++) {
v += *addr[i];
}


nanosleep(, NULL);

for (i = 0; i < nchunks; i++) {
munmap(addr[i], PAGE_SIZE * pagecount);
}
}
}

int main(int argc, char *argv[]) {
pthread_t thr[nthreads];
int i;

srandom(time(NULL));

if (argc < 2) {
fprintf(stderr, "usage: %s \n", argv[0]);
exit(1);
}

fd = open(argv[1], O_RDONLY);
if (fd < 0) {
fprintf(stderr, "open\n");
exit(1);
}

for (i = 0; i < nthreads; i++) {
if(pthread_create([i], NULL, threadf, NULL)) {
fprintf(stderr, "pthread_create\n");
exit(1);
}
}

for (i = 0; i < nthreads; i++) {
if(pthread_join(thr[i], NULL)) {
fprintf(stderr, "pthread_join\n");
exit(1);
}
}

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

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Gilad Ben-Yossef]

On Mon, Nov 6, 2017 at 10:37 AM, Tobin C. Harding  wrote:
> On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
>> Registers ioread/iowrite operations were done via macros,
>> sometime using a "magical" implicit parameter.
>>
>> Replace all register access with simple inline macros.
>>
>> Signed-off-by: Gilad Ben-Yossef 
>
> Hi,
>
> Nice work. I had a little trouble following this one. Perhaps you are
> doing more than one thing per patch, feel free to ignore me if I am
> wrong but it seems you are moving the macro definition of CC_REG to a
> different header, adding the new inline functions and doing some other
> change that I can't grok (commented on below).
>
> Perhaps this patch could be broken up.

Thank you Tobin.

The original macro that I am replacing had an assumption of a variable void *
cc_base being defined in the context of the macro being called, even though
it was not listed in the explicit parameter list of the macro.

The inline function that replace it instead takes an explicit
parameter a pointer to
struct ssi_drive data * , who has said cc_base as one of the fields.

As a result several function that took a void * cc_base parameter
(which is than only
used implicitly via the macro without ever being visibly referenced), now take
struct ssi_drive data * parameter instead which is passed explicitly
to the inline
function.

These seems to be the places you are referring to. They are cascading changes
resulting from the change in API between the macro and the inline
function that replaces it.

I imagine I can try to break that change to two patches but at least
in my mind this is artificial
and it is a single logical change.

Having said that, if you think otherwise and consider this
none-reviewable even after this
explanation let me know and  I'd be happy to break it down.

Many thanks,
Gilad




>
>> ---
>>  drivers/staging/ccree/cc_hal.h   | 33 --
>>  drivers/staging/ccree/cc_regs.h  | 35 
>>  drivers/staging/ccree/dx_reg_base_host.h | 25 --
>>  drivers/staging/ccree/ssi_driver.c   | 47 --
>>  drivers/staging/ccree/ssi_driver.h   | 20 +--
>>  drivers/staging/ccree/ssi_fips.c | 12 +++
>>  drivers/staging/ccree/ssi_pm.c   |  4 +--
>>  drivers/staging/ccree/ssi_request_mgr.c  | 57 
>> 
>>  drivers/staging/ccree/ssi_sysfs.c| 11 +++---
>>  9 files changed, 78 insertions(+), 166 deletions(-)
>>  delete mode 100644 drivers/staging/ccree/cc_hal.h
>>  delete mode 100644 drivers/staging/ccree/cc_regs.h
>>  delete mode 100644 drivers/staging/ccree/dx_reg_base_host.h
>>
>> diff --git a/drivers/staging/ccree/cc_hal.h b/drivers/staging/ccree/cc_hal.h
>> deleted file mode 100644
>> index eecc866..000
>> --- a/drivers/staging/ccree/cc_hal.h
>> +++ /dev/null
>> @@ -1,33 +0,0 @@
>> -/*
>> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
>> - *
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License version 2 as
>> - * published by the Free Software Foundation.
>> - *
>> - * This program is distributed in the hope that it will be useful,
>> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> - * GNU General Public License for more details.
>> - *
>> - * You should have received a copy of the GNU General Public License
>> - * along with this program; if not, see .
>> - */
>> -
>> -/* pseudo cc_hal.h for cc7x_perf_test_driver (to be able to include code 
>> from
>> - * CC drivers).
>> - */
>> -
>> -#ifndef __CC_HAL_H__
>> -#define __CC_HAL_H__
>> -
>> -#include 
>> -
>> -#define READ_REGISTER(_addr) ioread32((_addr))
>> -#define WRITE_REGISTER(_addr, _data)  iowrite32((_data), (_addr))
>> -
>> -#define CC_HAL_WRITE_REGISTER(offset, val) \
>> - WRITE_REGISTER(cc_base + (offset), val)
>> -#define CC_HAL_READ_REGISTER(offset) READ_REGISTER(cc_base + (offset))
>> -
>> -#endif
>> diff --git a/drivers/staging/ccree/cc_regs.h 
>> b/drivers/staging/ccree/cc_regs.h
>> deleted file mode 100644
>> index 2a8fc73..000
>> --- a/drivers/staging/ccree/cc_regs.h
>> +++ /dev/null
>> @@ -1,35 +0,0 @@
>> -/*
>> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
>> - *
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License version 2 as
>> - * published by the Free Software Foundation.
>> - *
>> - * This program is distributed in the hope that it will be useful,
>> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> - * GNU General Public License for more details.
>> - *
>> - * You should have received a copy of the GNU General 

Re: [PATCH v10 0/9] Hyper-V: paravirtualized remote TLB flushing and hypercall improvements

[Wanpeng Li]

2017-08-03 0:09 GMT+08:00 Vitaly Kuznetsov :
> Changes since v9:
> - Rebase to 4.13-rc3.
> - Drop PATCH1 as it was already taken by Greg to char-misc tree. There're no
>   functional dependencies on this patch so the series can go through a 
> different tree
>   (and it actually belongs to x86 if I got Ingo's comment right).
> - Add in missing void return type in PATCH1 [Colin King, Ingo Molnar, Greg KH]
> - A few minor fixes in what is now PATCH7: add pr_fmt, tiny style fix in
>   hyperv_flush_tlb_others() [Andy Shevchenko]
> - Fix "error: implicit declaration of function 'virt_to_phys'" in PATCH2
>   reported by kbuild test robot (#include )
> - Add Steven's 'Reviewed-by:' to PATCH9.
>
> Original description:
>
> Hyper-V supports hypercalls for doing local and remote TLB flushing and
> gives its guests hints when using hypercall is preferred. While doing
> hypercalls for local TLB flushes is probably not practical (and is not
> being suggested by modern Hyper-V versions) remote TLB flush with a
> hypercall brings significant improvement.
>
> To test the series I wrote a special 'TLB trasher': on a 16 vCPU guest I
> was creating 32 threads which were doing 10 mmap/munmaps each on some
> big file. Here are the results:
>
> Before:
> # time ./pthread_mmap ./randfile
> real3m33.118s
> user0m3.698s
> sys 3m16.624s
>
> After:
> # time ./pthread_mmap ./randfile
> real2m19.920s
> user0m2.662s
> sys 2m9.948s
>
> This series brings a number of small improvements along the way: fast
> hypercall implementation and using it for event signaling, rep hypercalls
> implementation, hyperv tracing subsystem (which only traces the newly added
> remote TLB flush for now).
>

Hi Vitaly,

Could you attach your benchmark? I'm interested in to try the
implementation in paravirt kvm.

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

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> Registers ioread/iowrite operations were done via macros,
> sometime using a "magical" implicit parameter.
> 
> Replace all register access with simple inline macros.
> 
> Signed-off-by: Gilad Ben-Yossef 

Hi,

Nice work. I had a little trouble following this one. Perhaps you are
doing more than one thing per patch, feel free to ignore me if I am
wrong but it seems you are moving the macro definition of CC_REG to a
different header, adding the new inline functions and doing some other
change that I can't grok (commented on below).

Perhaps this patch could be broken up.

> ---
>  drivers/staging/ccree/cc_hal.h   | 33 --
>  drivers/staging/ccree/cc_regs.h  | 35 
>  drivers/staging/ccree/dx_reg_base_host.h | 25 --
>  drivers/staging/ccree/ssi_driver.c   | 47 --
>  drivers/staging/ccree/ssi_driver.h   | 20 +--
>  drivers/staging/ccree/ssi_fips.c | 12 +++
>  drivers/staging/ccree/ssi_pm.c   |  4 +--
>  drivers/staging/ccree/ssi_request_mgr.c  | 57 
> 
>  drivers/staging/ccree/ssi_sysfs.c| 11 +++---
>  9 files changed, 78 insertions(+), 166 deletions(-)
>  delete mode 100644 drivers/staging/ccree/cc_hal.h
>  delete mode 100644 drivers/staging/ccree/cc_regs.h
>  delete mode 100644 drivers/staging/ccree/dx_reg_base_host.h
> 
> diff --git a/drivers/staging/ccree/cc_hal.h b/drivers/staging/ccree/cc_hal.h
> deleted file mode 100644
> index eecc866..000
> --- a/drivers/staging/ccree/cc_hal.h
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/* pseudo cc_hal.h for cc7x_perf_test_driver (to be able to include code from
> - * CC drivers).
> - */
> -
> -#ifndef __CC_HAL_H__
> -#define __CC_HAL_H__
> -
> -#include 
> -
> -#define READ_REGISTER(_addr) ioread32((_addr))
> -#define WRITE_REGISTER(_addr, _data)  iowrite32((_data), (_addr))
> -
> -#define CC_HAL_WRITE_REGISTER(offset, val) \
> - WRITE_REGISTER(cc_base + (offset), val)
> -#define CC_HAL_READ_REGISTER(offset) READ_REGISTER(cc_base + (offset))
> -
> -#endif
> diff --git a/drivers/staging/ccree/cc_regs.h b/drivers/staging/ccree/cc_regs.h
> deleted file mode 100644
> index 2a8fc73..000
> --- a/drivers/staging/ccree/cc_regs.h
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/*!
> - * @file
> - * @brief This file contains macro definitions for accessing ARM TrustZone
> - *CryptoCell register space.
> - */
> -
> -#ifndef _CC_REGS_H_
> -#define _CC_REGS_H_
> -
> -#include 
> -
> -#define AXIM_MON_COMP_VALUE GENMASK(DX_AXIM_MON_COMP_VALUE_BIT_SIZE + \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT, \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT)
> -
> -/* Register name mangling macro */
> -#define CC_REG(reg_name) DX_ ## reg_name ## _REG_OFFSET
> -
> -#endif /*_CC_REGS_H_*/
> diff --git a/drivers/staging/ccree/dx_reg_base_host.h 
> b/drivers/staging/ccree/dx_reg_base_host.h
> deleted file mode 100644
> index 47bbadb..000
> --- a/drivers/staging/ccree/dx_reg_base_host.h
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty 

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[gre...@linuxfoundation.org]

On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> On Mon, Nov 06, 2017 at 09:02:21AM +0100, gre...@linuxfoundation.org wrote:
> > On Mon, Nov 06, 2017 at 03:30:48PM +1100, Tobin C. Harding wrote:
> > > On Mon, Oct 02, 2017 at 05:49:52PM +0200, gre...@linuxfoundation.org 
> > > wrote:
> > > > On Mon, Oct 02, 2017 at 03:41:42PM +, Kershner, David A wrote:
> > > > > Hey Greg, we think the code for visorbus is ready to be moved out
> > > > > of staging, can you review it to see if we have missed anything?
> > > > 
> > > > I think your html email got rejected by the list :(
> > > > 
> > > > > The files include:
> > > > > /drivers/staging/unisys/visorbus/
> > > > > /drivers/staging/unisys/include/visorchannel.h
> > > > > /drivers/staging/unisys/include/visorbus.h
> > > > > 
> > > > > The directories staging/drivers/unisys/visornic, visorhba, and 
> > > > > visorinput
> > > > > are not part of the review as well as the file
> > > > > drivers/staging/unisys/include/iochannel.h.
> > > > > 
> > > > > We currently have 5 checkpatch.pl warnings that I know about:
> > > > >  - 3 CHECKs in visorchannel that are due to a MACRO that gets passed 
> > > > > a FIELD
> > > > > instead of just a variable
> > > > > - 2 WARNINGS dealing with char * becoming static const
> > > > > 
> > > > >  
> > > > > 
> > > > > Dan Carpenter found some extra parenthesis errors that I will address 
> > > > > as
> > > > > well as look through the code to fix, but I would like to ask for the 
> > > > > review
> > > > > while we are working on that.
> > > > 
> > > > Sure, I'll look at doing it in a week or so when I catch up with other
> > > > patches in my queue.
> > > > 
> > > > Everyone else is also welcome to do review :)
> > > 
> > > cppcheck emits a few style warnings, nothing super important but FWIW
> > > here is a patch
> > > 
> > > ---
> > >  drivers/staging/unisys/visorbus/visorchannel.c | 9 ++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/unisys/visorbus/visorchannel.c 
> > > b/drivers/staging/unisys/visorbus/visorchannel.c
> > > index aae16073ba03..2c1beddfee29 100644
> > > --- a/drivers/staging/unisys/visorbus/visorchannel.c
> > > +++ b/drivers/staging/unisys/visorbus/visorchannel.c
> > > @@ -131,12 +131,13 @@ int visorchannel_write(struct visorchannel 
> > > *channel, ulong offset, void *dest,
> > >  ulong nbytes)
> > >  {
> > >   size_t chdr_size = sizeof(struct channel_header);
> > > - size_t copy_size;
> > >  
> > >   if (offset + nbytes > channel->nbytes)
> > >   return -EIO;
> > >  
> > >   if (offset < chdr_size) {
> > > + size_t copy_size;
> > > +
> > 
> > Ick, no, the original code here is fine.
> > 
> > >   copy_size = min(chdr_size - offset, nbytes);
> > >   memcpy(((char *)(>chan_hdr)) + offset,
> > >  dest, copy_size);
> > > @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
> > > *channel, u32 queue,
> > > void *msg)
> > >  {
> > >   int rc;
> > > - unsigned long flags;
> > >  
> > >   if (channel->needs_lock) {
> > > + unsigned long flags;
> > > +
> > 
> > Same here, the original code is fine.
> > 
> > No idea why the tool wants you to move these around, you should ignore
> > stuff like that :(
> 
> Oh? I'm surprise at this comment. I have always thought limiting scope
> of local variables was seen as a good thing. Is this a style thing that
> is on case by case basis or do you never like to declare local variables
> within blocks?

It really doesn't matter as the compiler creates the same amount of
stack space (or used to, maybe newer versions are better, haven't looked
at it in a few years).

And functions shouldn't be all _that_ big that you need to limit the
scope of a local variable :)

thanks,

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

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:02:21AM +0100, gre...@linuxfoundation.org wrote:
> On Mon, Nov 06, 2017 at 03:30:48PM +1100, Tobin C. Harding wrote:
> > On Mon, Oct 02, 2017 at 05:49:52PM +0200, gre...@linuxfoundation.org wrote:
> > > On Mon, Oct 02, 2017 at 03:41:42PM +, Kershner, David A wrote:
> > > > Hey Greg, we think the code for visorbus is ready to be moved out
> > > > of staging, can you review it to see if we have missed anything?
> > > 
> > > I think your html email got rejected by the list :(
> > > 
> > > > The files include:
> > > > /drivers/staging/unisys/visorbus/
> > > > /drivers/staging/unisys/include/visorchannel.h
> > > > /drivers/staging/unisys/include/visorbus.h
> > > > 
> > > > The directories staging/drivers/unisys/visornic, visorhba, and 
> > > > visorinput
> > > > are not part of the review as well as the file
> > > > drivers/staging/unisys/include/iochannel.h.
> > > > 
> > > > We currently have 5 checkpatch.pl warnings that I know about:
> > > >  - 3 CHECKs in visorchannel that are due to a MACRO that gets passed a 
> > > > FIELD
> > > > instead of just a variable
> > > > - 2 WARNINGS dealing with char * becoming static const
> > > > 
> > > >  
> > > > 
> > > > Dan Carpenter found some extra parenthesis errors that I will address as
> > > > well as look through the code to fix, but I would like to ask for the 
> > > > review
> > > > while we are working on that.
> > > 
> > > Sure, I'll look at doing it in a week or so when I catch up with other
> > > patches in my queue.
> > > 
> > > Everyone else is also welcome to do review :)
> > 
> > cppcheck emits a few style warnings, nothing super important but FWIW
> > here is a patch
> > 
> > ---
> >  drivers/staging/unisys/visorbus/visorchannel.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/unisys/visorbus/visorchannel.c 
> > b/drivers/staging/unisys/visorbus/visorchannel.c
> > index aae16073ba03..2c1beddfee29 100644
> > --- a/drivers/staging/unisys/visorbus/visorchannel.c
> > +++ b/drivers/staging/unisys/visorbus/visorchannel.c
> > @@ -131,12 +131,13 @@ int visorchannel_write(struct visorchannel *channel, 
> > ulong offset, void *dest,
> >ulong nbytes)
> >  {
> > size_t chdr_size = sizeof(struct channel_header);
> > -   size_t copy_size;
> >  
> > if (offset + nbytes > channel->nbytes)
> > return -EIO;
> >  
> > if (offset < chdr_size) {
> > +   size_t copy_size;
> > +
> 
> Ick, no, the original code here is fine.
> 
> > copy_size = min(chdr_size - offset, nbytes);
> > memcpy(((char *)(>chan_hdr)) + offset,
> >dest, copy_size);
> > @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
> > *channel, u32 queue,
> >   void *msg)
> >  {
> > int rc;
> > -   unsigned long flags;
> >  
> > if (channel->needs_lock) {
> > +   unsigned long flags;
> > +
> 
> Same here, the original code is fine.
> 
> No idea why the tool wants you to move these around, you should ignore
> stuff like that :(

Oh? I'm surprise at this comment. I have always thought limiting scope
of local variables was seen as a good thing. Is this a style thing that
is on case by case basis or do you never like to declare local variables
within blocks?

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

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[gre...@linuxfoundation.org]

On Mon, Nov 06, 2017 at 03:30:48PM +1100, Tobin C. Harding wrote:
> On Mon, Oct 02, 2017 at 05:49:52PM +0200, gre...@linuxfoundation.org wrote:
> > On Mon, Oct 02, 2017 at 03:41:42PM +, Kershner, David A wrote:
> > > Hey Greg, we think the code for visorbus is ready to be moved out
> > > of staging, can you review it to see if we have missed anything?
> > 
> > I think your html email got rejected by the list :(
> > 
> > > The files include:
> > > /drivers/staging/unisys/visorbus/
> > > /drivers/staging/unisys/include/visorchannel.h
> > > /drivers/staging/unisys/include/visorbus.h
> > > 
> > > The directories staging/drivers/unisys/visornic, visorhba, and visorinput
> > > are not part of the review as well as the file
> > > drivers/staging/unisys/include/iochannel.h.
> > > 
> > > We currently have 5 checkpatch.pl warnings that I know about:
> > >  - 3 CHECKs in visorchannel that are due to a MACRO that gets passed a 
> > > FIELD
> > > instead of just a variable
> > > - 2 WARNINGS dealing with char * becoming static const
> > > 
> > >  
> > > 
> > > Dan Carpenter found some extra parenthesis errors that I will address as
> > > well as look through the code to fix, but I would like to ask for the 
> > > review
> > > while we are working on that.
> > 
> > Sure, I'll look at doing it in a week or so when I catch up with other
> > patches in my queue.
> > 
> > Everyone else is also welcome to do review :)
> 
> cppcheck emits a few style warnings, nothing super important but FWIW
> here is a patch
> 
> ---
>  drivers/staging/unisys/visorbus/visorchannel.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchannel.c 
> b/drivers/staging/unisys/visorbus/visorchannel.c
> index aae16073ba03..2c1beddfee29 100644
> --- a/drivers/staging/unisys/visorbus/visorchannel.c
> +++ b/drivers/staging/unisys/visorbus/visorchannel.c
> @@ -131,12 +131,13 @@ int visorchannel_write(struct visorchannel *channel, 
> ulong offset, void *dest,
>  ulong nbytes)
>  {
>   size_t chdr_size = sizeof(struct channel_header);
> - size_t copy_size;
>  
>   if (offset + nbytes > channel->nbytes)
>   return -EIO;
>  
>   if (offset < chdr_size) {
> + size_t copy_size;
> +

Ick, no, the original code here is fine.

>   copy_size = min(chdr_size - offset, nbytes);
>   memcpy(((char *)(>chan_hdr)) + offset,
>  dest, copy_size);
> @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
> *channel, u32 queue,
> void *msg)
>  {
>   int rc;
> - unsigned long flags;
>  
>   if (channel->needs_lock) {
> + unsigned long flags;
> +

Same here, the original code is fine.

No idea why the tool wants you to move these around, you should ignore
stuff like that :(

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