Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()
On Sun, Dec 14, 2014 at 11:59:19PM -0800, Jeremiah Mahler wrote: > KY Srinivasan, > > On Mon, Dec 15, 2014 at 07:00:45AM +, KY Srinivasan wrote: > > > > > > > -Original Message- > > > From: Jeremiah Mahler [mailto:jmmah...@gmail.com] > > > Sent: Wednesday, December 10, 2014 6:10 PM > > > To: KY Srinivasan > > > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > > > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > > > jasow...@redhat.com; mc...@ipxe.org > > > Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in > > > vmbus_establish_gpadl() > > > > > > K. Y. Srinivasan, > > > > > > On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote: > > > > Correctly compute the local (gpadl) handle. > > > > > > This description is still too sparse for me. How was it computed before > > > and > > > why was this incorrect? Pretend like you are trying to explain your > > > patch to > > > someone who has no idea what you did. > > > > > > > I would like to thank Michael Brown for seeing this > > > > bug. > > > > > > > > Signed-off-by: K. Y. Srinivasan > > > > Reported-by: Michael Brown > > > > --- > > > > Changes in V2: Added the Reported-by tag. > > > > Changes in V3: Cleaned up the commit log. > > > > > > > > drivers/hv/channel.c |4 ++-- > > > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index > > > > 433f72a..c76ffbe 100644 > > > > --- a/drivers/hv/channel.c > > > > +++ b/drivers/hv/channel.c > > > > @@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel > > > *channel, void *kbuffer, > > > > unsigned long flags; > > > > int ret = 0; > > > > > > > > - next_gpadl_handle = > > > atomic_read(&vmbus_connection.next_gpadl_handle); > > > > - atomic_inc(&vmbus_connection.next_gpadl_handle); > > > > + next_gpadl_handle = > > > > + > > > (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1); > > > > > > > Tell me if I understand this correctly. > > > > > > Before it read the handle and incremented it. > > > > > > y = x + 1 > > > > > > Now it reads the handle, increments it, then decrements it. > > > > > > y = (x + 1) - 1 = x > > > > This code can be executed concurrently on multiple CPUs. We want to ensure > > that each call to > > establish gpadl gets a unique local handle. The earlier code was buggy in > > that we would read the > > handle and then atomically increment it. Thus, multiple CPUs could read the > > identical current > > value which would be their local handle. What we want is the ability to > > atomically read and increment > > the value - this would ensure that each caller got a unique value even if > > they executed the code > > concurrently on multiple CPUs. The API atomic_inc_return(), atomically > > increments and returns the > > incremented value. We locally decrement this value to emulate the logic of > > "read the current value and > > atomically increment the value. > > > > Hope this helps, > > > > K. Y > > > > [...] > > So to avoid concurrency issues you used a single atomic operation > instead of two separate operations. That make sense. But it still > doesn't explain why you changed the calculation by subtracting 1. The calculation appears identical to my reading, the original form was: next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle); atomic_inc(&vmbus_connection.next_gpadl_handle); or: y = x; x++; so y == x' (x before incrementing) the new code is: next_gpadl_handle = (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1); or: y = ++x - 1; Also making y = x' (x before incrementing) -apw ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] quiet checkpatch style recommendation about no spaces around bitfield :
On Mon, Mar 31, 2014 at 01:56:28PM -0700, Andrew Morton wrote:
> On Mon, 31 Mar 2014 08:31:38 -0700 Joe Perches wrote:
>
> > > > @@ -143,13 +143,13 @@ union cvmx_usbcx_gahbcfg {
> > > > * * 1'b1: Unmask the interrupt assertion to the
> > > > application.
> > > > */
> > > > struct cvmx_usbcx_gahbcfg_s {
> > > > - uint32_t reserved_9_31 : 23;
> > > > - uint32_t ptxfemplvl : 1;
> > > > - uint32_t nptxfemplvl: 1;
> > > > - uint32_t reserved_6_6 : 1;
> > > > - uint32_t dmaen : 1;
> > > > - uint32_t hbstlen: 4;
> > > > - uint32_t glblintrmsk: 1;
> > > > + uint32_t reserved_9_31:23;
> > > > + uint32_t ptxfemplvl:1;
> > > > + uint32_t nptxfemplvl:1;
> > > > + uint32_t reserved_6_6:1;
> > > > + uint32_t dmaen:1;
> > > > + uint32_t hbstlen:4;
> > > > + uint32_t glblintrmsk:1;
> > > > } s;
> > > The warning here is:
> > > ERROR: spaces prohibited around that ':' (ctx:WxW)
> > > I have done a kernel wide search for these warnings
> >
> > Really? How?
> >
> > > and I think we
> > > should disable this warning. It has too many false positives like this.
> >
> > It does seem a bit noisy to me too.
> >
> > Andy? Andrew?
>
> I suspect we don't use bitfields enough for a particular style to have
> emerged. Personally I think the above patch made the code harder to
> read, so...
Yeah it seems the predominant style would trigger the warning. So turn
it off.
-apw
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/2] scsi: hyper-v storvsc changes by Ubuntu
On 16 May 2014 16:39, Ian Abbott wrote: > These changes to the Microsoft Hyper-V storage driver in Ubuntu Saucy's > 3.13 kernel look useful for the mainline kernel, especially as they > enable 'TRIM' support. > > Andy Whitcroft (2): > scsi: hyper-v storvsc switch up to SPC-3 > scsi: hyper-v storvsc -- host takes MAINTENANCE_IN commands badly -- > elide them > > drivers/scsi/storvsc_drv.c | 10 ++ > 1 file changed, 10 insertions(+) The back story here is a little complex. The main issue is that the Hyper-V drives claim to be SPC-2, and yet implement the SPC-3 extensions for TRIM. We did attempt to upstream quirks to allow these features to be negotiated specifically for the Hyper-V virtual drives (minimum regression potential) but these were NAKd, and it was suggested that just overriding the Hyper-V drives to SPC-3 unconditionally was more appropriate. The first of the patches here does does this. This has been sitting in our tree for some time as it was not clear that this would be entirely safe, though the SPC-3 bits are in theory at least mostly detected. That said this change has been in Ubuntu for a full cycle now and does not seem to have caused any issues. If KY is happy we should likely submit it formally. The second fix I believed was already being submitted to mainline. KY? -apw ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/4] Hyper-V TRIM support
tl;dr -- enable TRIM support for Hyper-V emulated disks. The Hyper-V hypervisor can support TRIM for its devices, advertising this via the appropriate VPD pages. However the emulated disks only claim to be SPC-2 devices. According to the specs VPD pages (in general) did exist at SPC-2 but the specific pages we interogate for the TRIM support did not until SPC-3 therefore the kernel avoids reading the relevant pages for SPC-2 devices and prevents TRIM from being offered for these devices. Additionally at SPC-2 we prefer ReadCapacity10 over ReadCapacity16 and unless we use RC16 we will not identify the device as TRIM capable also preventing TRIM being offered. As the VPD page zero does list which pages are valid for each device, it could be argued that we could simply attempt to use these pages for all devices which claim to be SPC-2 and above. While this seems valid the code documents a number of devices which take badly to having even VPD page 0 interogated even when supposedly supported. Therefore it seems appropriate to add a scsi device flag to allow a device to request SPC-3 VPD pages be used when only at SPC-2. Similarly for the ReadCapacity selection it seems dangerous to invert the order for all SPC-2 devices. So it seems appropriate to add a scsi device flag to request we try RC16 before RC10 (mirroring the existing flag for the opposite). The following four patches add the two scsi device flags and select those flags for the Hyper-V emulated disks. Patches against v3.11. Comments? -apw Andy Whitcroft (4): scsi: add scsi device flag to request VPD pages be used at SPC-2 scsi: add scsi device flag to request READ CAPACITY (16) be preferred scsi: hyper-v storage -- mark as VPD capable at SPC-2 scsi: hyper-v storage -- mark as preferring READ CAPACITY (16) at SPC-2 drivers/scsi/sd.c | 8 +++- drivers/scsi/storvsc_drv.c | 2 ++ include/scsi/scsi_device.h | 2 ++ 3 files changed, 11 insertions(+), 1 deletion(-) -- 1.8.1.2 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/4] scsi: hyper-v storage -- mark as VPD capable at SPC-2
BugLink: http://bugs.launchpad.net/bugs/1223499 Signed-off-by: Andy Whitcroft --- drivers/scsi/storvsc_drv.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index 1a28f56..14ba8fd 100644 --- a/drivers/scsi/storvsc_drv.c +++ b/drivers/scsi/storvsc_drv.c @@ -1437,6 +1437,7 @@ static int storvsc_device_configure(struct scsi_device *sdevice) blk_queue_rq_timeout(sdevice->request_queue, (storvsc_timeout * HZ)); sdevice->no_write_same = 1; + sdevice->use_vpd_spc2 = 1; return 0; } -- 1.8.1.2 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/4] scsi: add scsi device flag to request READ CAPACITY (16) be preferred
BugLink: http://bugs.launchpad.net/bugs/1223499
Signed-off-by: Andy Whitcroft
---
drivers/scsi/sd.c | 2 ++
include/scsi/scsi_device.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a8a04d..eba4d6c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2104,6 +2104,8 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
return 1;
if (scsi_device_protection(sdp))
return 1;
+ if (sdp->try_rc_16_first)
+ return 1;
return 0;
}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9b7fdb6..df3ed43 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -165,6 +165,7 @@ struct scsi_device {
unsigned no_read_disc_info:1; /* Avoid READ_DISC_INFO cmds */
unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
unsigned try_rc_10_first:1; /* Try READ_CAPACACITY_10 first */
+ unsigned try_rc_16_first:1; /* Try READ_CAPACACITY_16 first */
unsigned is_visible:1; /* is the device visible in sysfs */
unsigned wce_default_on:1; /* Cache is ON by default */
unsigned no_dif:1; /* T10 PI (DIF) should be disabled */
--
1.8.1.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/4] scsi: hyper-v storage -- mark as preferring READ CAPACITY (16) at SPC-2
BugLink: http://bugs.launchpad.net/bugs/1223499 Signed-off-by: Andy Whitcroft --- drivers/scsi/storvsc_drv.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index 14ba8fd..25e7dd5 100644 --- a/drivers/scsi/storvsc_drv.c +++ b/drivers/scsi/storvsc_drv.c @@ -1438,6 +1438,7 @@ static int storvsc_device_configure(struct scsi_device *sdevice) sdevice->no_write_same = 1; sdevice->use_vpd_spc2 = 1; + sdevice->try_rc_16_first = 1; return 0; } -- 1.8.1.2 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/4] scsi: add scsi device flag to request VPD pages be used at SPC-2
Under Hyper-V the disk devices support the trim extensions advertising them
via the appropriate VPD pages, it however reports itself as SPC-2 only.
The relevant pages were added in SPC-3 and later, so we do not even
attempt to see if they are present; the VPD page 0 lists which other
pages are present.
Add a scsi quirk bit to allow those devices to hint we can and should
try these VPD pages.
BugLink: http://bugs.launchpad.net/bugs/1223499
Signed-off-by: Andy Whitcroft
---
drivers/scsi/sd.c | 6 +-
include/scsi/scsi_device.h | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b58e8f8..5a8a04d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2667,8 +2667,12 @@ static int sd_try_extended_inquiry(struct scsi_device
*sdp)
* Although VPD inquiries can go to SCSI-2 type devices,
* some USB ones crash on receiving them, and the pages
* we currently ask for are for SPC-3 and beyond
+* Allow devices to request use of VPD inquiry at SPC-2
+* as some devices implement these extensions (inc Hyper-V)
*/
- if (sdp->scsi_level > SCSI_SPC_2 && !sdp->skip_vpd_pages)
+ if ((sdp->scsi_level > SCSI_SPC_2 ||
+(sdp->use_vpd_spc2 && sdp->scsi_level >= SCSI_SPC_2)) &&
+ !sdp->skip_vpd_pages)
return 1;
return 0;
}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d65fbec..9b7fdb6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -149,6 +149,7 @@ struct scsi_device {
unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08 */
unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f */
unsigned skip_vpd_pages:1; /* do not read VPD pages */
+ unsigned use_vpd_spc2:1;/* do use VPD pages at SPC-2 */
unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
unsigned no_start_on_add:1; /* do not issue start on add */
unsigned allow_restart:1; /* issue START_UNIT in error handler */
--
1.8.1.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/4] Hyper-V TRIM support
On Fri, Sep 13, 2013 at 07:57:58AM -0700, James Bottomley wrote: > This is an awful lot of contortions (which don't seem to have any other > users on the horizon) to support a device that's not standards > compliant. What about this, it's simple, it does the right thing and > it's contained in the driver of the problem device. > > James > > --- > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c > index 83ec1aa..bd86a4b 100644 > --- a/drivers/scsi/storvsc_drv.c > +++ b/drivers/scsi/storvsc_drv.c > @@ -1438,6 +1438,12 @@ static int storvsc_device_configure(struct scsi_device > *sdevice) > > sdevice->no_write_same = 1; > > + /* > + * hyper-v is stupid and lies about its capabilities > + * If we pretend to be SPC-3, we send RC16 which activates trim > + */ > + sdevice->scsi_level = SCSI_SPC_3; > + > return 0; > } I will get that tested and see if there are any other negative side effects. -apw ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
