Re: [patch 47/54] Staging: hv: make gVmbusConnection.ChannelLock a real spinlock

Re: [patch 47/54] Staging: hv: make gVmbusConnection.ChannelLock a real spinlock

[Arnd Bergmann]

On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> I don't think we really want to be doing a irqsave for this code, but I
> left it alone to preserve the original codepath.  It should be reviewed
> later.

This patch does not preserve the original code path if any of the
callers has a different irq state from the others. When flags is
shared, releasing the lock actually sets the irq state to whatever
another thread was using when failing to acquire the lock while
it was held.

The patch of course looks good, but the comment is misleading.

	Arnd <><

Re: [patch 47/54] Staging: hv: make gVmbusConnection.ChannelLock a real spinlock

[Greg KH]

On Tue, Jul 21, 2009 at 01:39:19AM +0200, Arnd Bergmann wrote:
> On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> > I don't think we really want to be doing a irqsave for this code, but I
> > left it alone to preserve the original codepath.  It should be reviewed
> > later.
> 
> This patch does not preserve the original code path if any of the
> callers has a different irq state from the others. When flags is
> shared, releasing the lock actually sets the irq state to whatever
> another thread was using when failing to acquire the lock while
> it was held.
> 
> The patch of course looks good, but the comment is misleading.

Good point, sorry for the misleading comment.

I'll let Hank work out the details as to if the irqsave mode is needed
or not :)

thanks,

greg k-h

Re: [patch 34/54] Staging: hv: remove STRUCT_PACKED and STRUCT_ALIGNED defines

[Arnd Bergmann]

On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> @@ -43,7 +43,7 @@ typedef struct _RING_BUFFER {
>         // volatile u32 InterruptMask;
>         // Ring data starts here + RingDataStartOffset !!! DO NOT place any fields below this !!!
>      u8         Buffer[0];
> -} STRUCT_PACKED RING_BUFFER;
> +} __attribute__((packed)) RING_BUFFER;
>  

The data structure is actually packed already, the attribute does not make it better
and could be removed. We also have __packed as a shortcut for __attribute__((packed)).

	Arnd <><

Re: [patch 34/54] Staging: hv: remove STRUCT_PACKED and STRUCT_ALIGNED defines

[Jörn Engel]

On Tue, 21 July 2009 01:46:41 +0200, Arnd Bergmann wrote:
> On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> > @@ -43,7 +43,7 @@ typedef struct _RING_BUFFER {
> >         // volatile u32 InterruptMask;
> >         // Ring data starts here + RingDataStartOffset !!! DO NOT place any fields below this !!!
> >      u8         Buffer[0];
> > -} STRUCT_PACKED RING_BUFFER;
> > +} __attribute__((packed)) RING_BUFFER;
> >  
> 
> The data structure is actually packed already, the attribute does not make it better
> and could be removed. We also have __packed as a shortcut for __attribute__((packed)).

Honestly, I don't know how useful __packed really is.  In a shared
kernel/userspace header, it is only defined for the kernel.

Jörn

-- 
Do not stop an army on its way home.
-- Sun Tzu

Re: [patch 34/54] Staging: hv: remove STRUCT_PACKED and STRUCT_ALIGNED defines

[Stephen Hemminger]

On Fri, 24 Jul 2009 23:32:19 +0200
Jörn Engel <joern@logfs.org> wrote:

> On Tue, 21 July 2009 01:46:41 +0200, Arnd Bergmann wrote:
> > On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> > > @@ -43,7 +43,7 @@ typedef struct _RING_BUFFER {
> > >         // volatile u32 InterruptMask;
> > >         // Ring data starts here + RingDataStartOffset !!! DO NOT place any fields below this !!!
> > >      u8         Buffer[0];
> > > -} STRUCT_PACKED RING_BUFFER;
> > > +} __attribute__((packed)) RING_BUFFER;
> > >  
> > 
> > The data structure is actually packed already, the attribute does not make it better
> > and could be removed. We also have __packed as a shortcut for __attribute__((packed)).
> 
> Honestly, I don't know how useful __packed really is.  In a shared
> kernel/userspace header, it is only defined for the kernel.
> 

As I remember, gcc generates worse code for packed structures on many architectures
since it may have to do byte fetchs/recombining to avoid unaligned
accesses.

-- 

Re: [patch 34/54] Staging: hv: remove STRUCT_PACKED and STRUCT_ALIGNED defines

[Jörn Engel]

On Fri, 24 July 2009 14:50:25 -0700, Stephen Hemminger wrote:
> On Fri, 24 Jul 2009 23:32:19 +0200
> Jörn Engel <joern@logfs.org> wrote:
> 
> > On Tue, 21 July 2009 01:46:41 +0200, Arnd Bergmann wrote:
> > > 
> > > The data structure is actually packed already, the attribute does not make it better
> > > and could be removed. We also have __packed as a shortcut for __attribute__((packed)).
> > 
> > Honestly, I don't know how useful __packed really is.  In a shared
> > kernel/userspace header, it is only defined for the kernel.
> 
> As I remember, gcc generates worse code for packed structures on many architectures
> since it may have to do byte fetchs/recombining to avoid unaligned
> accesses.

I was talking about "__packed" vs. "__attribute__((packed))".  But yes,
avoiding packed structures where possible is a good idea.

Jörn

-- 
"[One] doesn't need to know [...] how to cause a headache in order
to take an aspirin."
-- Scott Culp, Manager of the Microsoft Security Response Center, 2001

Re: [patch 27/54] Staging: hv: remove ULONG and LONG typedefs

[Arnd Bergmann]

On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@suse.de>
> 
> The ULONG and LONG typedefs are now removed from the Hyper-V driver
> code
> 

This conversion is not equivalent:

>  //
> -// unsigned types
> -//
> -typedef unsigned int		ULONG;
> -
> -//
> -// signed types
> -//
> -typedef int					LONG;

- // redefine the world, because we can
- extern char *i;
- void abs(unsigned u);
- #define FALSE (2)

;-)

> --- a/drivers/staging/hv/include/StorVscApi.h
> +++ b/drivers/staging/hv/include/StorVscApi.h
> @@ -117,7 +117,7 @@ typedef struct _STORVSC_DRIVER_OBJECT {
>  } STORVSC_DRIVER_OBJECT;
>  
>  typedef struct _STORVSC_DEVICE_INFO {
> -	ULONG	PortNumber;
> +	unsigned long	PortNumber;
>      unsigned char	PathId;
>      unsigned char	TargetId;
>  } STORVSC_DEVICE_INFO;

If this is an API (as the header file name suggests), you just changed it.
Same for the other hunks in this patch.

	Arnd <><

RE: [patch 27/54] Staging: hv: remove ULONG and LONG typedefs

[Hank Janssen]

Arnd,

Thanks for this and the other mails reviewing the code.

My TODO list keeps on getting bigger :). I will check into all of these and correct them.

Keep them coming!

Thanks!

Hank.


-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de]
Sent: Monday, July 20, 2009 5:00 PM
To: virtualization@lists.linux-foundation.org
Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org; virtualization@lists.osdl.org; Greg KH; Sam Ramji; Haiyang Zhang; Hank Janssen; shemminger@linux-foundation.org
Subject: Re: [patch 27/54] Staging: hv: remove ULONG and LONG typedefs

On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@suse.de>
>
> The ULONG and LONG typedefs are now removed from the Hyper-V driver
> code
>

This conversion is not equivalent:

>  //
> -// unsigned types
> -//
> -typedef unsigned int         ULONG;
> -
> -//
> -// signed types
> -//
> -typedef int                                  LONG;

- // redefine the world, because we can
- extern char *i;
- void abs(unsigned u);
- #define FALSE (2)

;-)

> --- a/drivers/staging/hv/include/StorVscApi.h
> +++ b/drivers/staging/hv/include/StorVscApi.h
> @@ -117,7 +117,7 @@ typedef struct _STORVSC_DRIVER_OBJECT {
>  } STORVSC_DRIVER_OBJECT;
>
>  typedef struct _STORVSC_DEVICE_INFO {
> -     ULONG   PortNumber;
> +     unsigned long   PortNumber;
>      unsigned char    PathId;
>      unsigned char    TargetId;
>  } STORVSC_DEVICE_INFO;

If this is an API (as the header file name suggests), you just changed it.
Same for the other hunks in this patch.

        Arnd <><

Re: [patch 27/54] Staging: hv: remove ULONG and LONG typedefs

[Greg KH]

On Tue, Jul 21, 2009 at 02:00:16AM +0200, Arnd Bergmann wrote:
> On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> > From: Greg Kroah-Hartman <gregkh@suse.de>
> > 
> > The ULONG and LONG typedefs are now removed from the Hyper-V driver
> > code
> > 
> 
> This conversion is not equivalent:
> 
> >  //
> > -// unsigned types
> > -//
> > -typedef unsigned int		ULONG;
> > -
> > -//
> > -// signed types
> > -//
> > -typedef int					LONG;
> 
> - // redefine the world, because we can
> - extern char *i;
> - void abs(unsigned u);
> - #define FALSE (2)
> 
> ;-)
> 
> > --- a/drivers/staging/hv/include/StorVscApi.h
> > +++ b/drivers/staging/hv/include/StorVscApi.h
> > @@ -117,7 +117,7 @@ typedef struct _STORVSC_DRIVER_OBJECT {
> >  } STORVSC_DRIVER_OBJECT;
> >  
> >  typedef struct _STORVSC_DEVICE_INFO {
> > -	ULONG	PortNumber;
> > +	unsigned long	PortNumber;
> >      unsigned char	PathId;
> >      unsigned char	TargetId;
> >  } STORVSC_DEVICE_INFO;
> 
> If this is an API (as the header file name suggests), you just changed it.
> Same for the other hunks in this patch.

Ah crap, you're right, that's all wrong, nice catch.

I'll go redo it to be "unsigned int", but for some reason, the code
still seems to work properly :)

thanks,

greg k-h

Re: [patch 27/54] Staging: hv: remove ULONG and LONG typedefs

[Greg KH]

On Mon, Jul 20, 2009 at 07:38:23PM -0700, Greg KH wrote:
> On Tue, Jul 21, 2009 at 02:00:16AM +0200, Arnd Bergmann wrote:
> > On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> > > From: Greg Kroah-Hartman <gregkh@suse.de>
> > > 
> > > The ULONG and LONG typedefs are now removed from the Hyper-V driver
> > > code
> > > 
> > 
> > This conversion is not equivalent:
> > 
> > >  //
> > > -// unsigned types
> > > -//
> > > -typedef unsigned int		ULONG;
> > > -
> > > -//
> > > -// signed types
> > > -//
> > > -typedef int					LONG;
> > 
> > - // redefine the world, because we can
> > - extern char *i;
> > - void abs(unsigned u);
> > - #define FALSE (2)
> > 
> > ;-)
> > 
> > > --- a/drivers/staging/hv/include/StorVscApi.h
> > > +++ b/drivers/staging/hv/include/StorVscApi.h
> > > @@ -117,7 +117,7 @@ typedef struct _STORVSC_DRIVER_OBJECT {
> > >  } STORVSC_DRIVER_OBJECT;
> > >  
> > >  typedef struct _STORVSC_DEVICE_INFO {
> > > -	ULONG	PortNumber;
> > > +	unsigned long	PortNumber;
> > >      unsigned char	PathId;
> > >      unsigned char	TargetId;
> > >  } STORVSC_DEVICE_INFO;
> > 
> > If this is an API (as the header file name suggests), you just changed it.
> > Same for the other hunks in this patch.
> 
> Ah crap, you're right, that's all wrong, nice catch.
> 
> I'll go redo it to be "unsigned int", but for some reason, the code
> still seems to work properly :)

Ok, here's the updated version.  Thanks for the review.

Hank, a lot of these should probably be converted to "real" kernel
types, like u32, or u64 if it's really needed.  Especially if they are
crossing the kernel/hypervisor boundry.

thanks,

greg k-h

-------------
From: Greg Kroah-Hartman <gregkh@suse.de>
Subject: Staging: hv: remove ULONG and LONG typedefs

From: Greg Kroah-Hartman <gregkh@suse.de>

The ULONG and LONG typedefs are now removed from the Hyper-V driver
code.

Cc: Hank Janssen <hjanssen@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/staging/hv/StorVsc.c            |    2 +-
 drivers/staging/hv/include/StorVscApi.h |    2 +-
 drivers/staging/hv/include/osd.h        |   10 ----------
 drivers/staging/hv/include/vstorage.h   |   12 ++++++------
 4 files changed, 8 insertions(+), 18 deletions(-)

--- a/drivers/staging/hv/StorVsc.c
+++ b/drivers/staging/hv/StorVsc.c
@@ -62,7 +62,7 @@ typedef struct _STORVSC_DEVICE{
 
 	//  Each unique Port/Path/Target represents 1 channel ie scsi controller. In reality, the pathid, targetid is always 0
 	// and the port is set by us
-	ULONG						PortNumber;
+	unsigned int						PortNumber;
     unsigned char						PathId;
     unsigned char						TargetId;
 
--- a/drivers/staging/hv/include/StorVscApi.h
+++ b/drivers/staging/hv/include/StorVscApi.h
@@ -117,7 +117,7 @@ typedef struct _STORVSC_DRIVER_OBJECT {
 } STORVSC_DRIVER_OBJECT;
 
 typedef struct _STORVSC_DEVICE_INFO {
-	ULONG	PortNumber;
+	unsigned int	PortNumber;
     unsigned char	PathId;
     unsigned char	TargetId;
 } STORVSC_DEVICE_INFO;
--- a/drivers/staging/hv/include/osd.h
+++ b/drivers/staging/hv/include/osd.h
@@ -95,16 +95,6 @@ typedef struct _DLIST_ENTRY {
 } DLIST_ENTRY;
 
 //
-// unsigned types
-//
-typedef unsigned int		ULONG;
-
-//
-// signed types
-//
-typedef int					LONG;
-
-//
 // Other types
 //
 typedef unsigned long		SIZE_T;
--- a/drivers/staging/hv/include/vstorage.h
+++ b/drivers/staging/hv/include/vstorage.h
@@ -155,7 +155,7 @@ typedef struct
     unsigned char DataIn;
     unsigned char Reserved;
 
-    ULONG DataTransferLength;
+    unsigned int DataTransferLength;
 
     union
     {
@@ -185,11 +185,11 @@ typedef struct
     //
     // Note: port number is only really known on the client side
     //
-    ULONG  PortNumber;
+    unsigned int  PortNumber;
 
-    ULONG  Flags;
+    unsigned int  Flags;
 
-    ULONG  MaxTransferBytes;
+    unsigned int  MaxTransferBytes;
 
     //
     //  This id is unique for each channel and will correspond with
@@ -249,13 +249,13 @@ typedef struct _VSTOR_PACKET
     //  Flags - see below for values
     //
 
-    ULONG     Flags;
+    unsigned int     Flags;
 
     //
     // Status of the request returned from the server side.
     //
 
-    ULONG     Status;
+    unsigned int     Status;
 
     //
     // Data payload area

Re: [patch 27/54] Staging: hv: remove ULONG and LONG typedefs

[Bernd Petrovitsch]

On Tue, 2009-07-21 at 02:00 +0200, Arnd Bergmann wrote:
> On Friday 17 July 2009, Greg Kroah-Hartman wrote:
> > From: Greg Kroah-Hartman <gregkh@suse.de>
> > 
> > The ULONG and LONG typedefs are now removed from the Hyper-V driver
> > code
> 
> This conversion is not equivalent:
[...]
> - // redefine the world, because we can
[...]
> - #define FALSE (2)
That has been done already by the SNMP folks - see
http://www.faqs.org/rfcs/rfc1903.html under "TruthValue".
S,CNR and for off-topic ...

> ;-)
;-) either ....

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services