RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver

2011-07-16 Thread KY Srinivasan 


 -Original Message-
 From: Christoph Hellwig [mailto:h...@infradead.org]
 Sent: Friday, July 15, 2011 10:05 PM
 To: KY Srinivasan
 Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
 de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
 Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE 
 devices
 using the storvsc driver
 
 Thanks, this looks much cleaner than the initial variant.
 
  +   if (dev_is_ide) {
  +   storvsc_get_ide_info(device, target, path);
  +   host_dev-path = device_info.path_id;
  +   host_dev-target = device_info.target_id;
  +   } else {
  +   host_dev-path = device_info.path_id;
  +   host_dev-target = device_info.target_id;
  +   }
 
 Is using the device_info values in both branches intentional?  If so
 there's no need to have these assignments duplicated.

While we set the values in both the branches, the value set is different;
The IDE side encodes the bits differently and is appropriately parsed in the
function storvsc_get_ide_info().

Regards,

K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver

2011-07-16 Thread KY Srinivasan 


 -Original Message-
 From: Sasha Levin [mailto:levinsasha...@gmail.com]
 Sent: Saturday, July 16, 2011 9:02 AM
 To: KY Srinivasan
 Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org;
 de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
 Subject: RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE 
 devices
 using the storvsc driver
 
 On Sat, 2011-07-16 at 12:57 +, KY Srinivasan wrote:
 
   -Original Message-
   From: Christoph Hellwig [mailto:h...@infradead.org]
   Sent: Friday, July 15, 2011 10:05 PM
   To: KY Srinivasan
   Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
   de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
   Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
 devices
   using the storvsc driver
  
   Thanks, this looks much cleaner than the initial variant.
  
+   if (dev_is_ide) {
+   storvsc_get_ide_info(device, target, path);
+   host_dev-path = device_info.path_id;
+   host_dev-target = device_info.target_id;
+   } else {
+   host_dev-path = device_info.path_id;
+   host_dev-target = device_info.target_id;
+   }
  
   Is using the device_info values in both branches intentional?  If so
   there's no need to have these assignments duplicated.
 
  While we set the values in both the branches, the value set is different;
  The IDE side encodes the bits differently and is appropriately parsed in the
  function storvsc_get_ide_info().
 
 Is think that what Christoph meant was simplifying it to:
 
   if (dev_is_ide)
   storvsc_get_ide_info(device, target, path);
 
   host_dev-path = device_info.path_id;
   host_dev-target = device_info.target_id;

Good point! It is still too early in the morning for me. 
Greg, if it is ok with you, in the interest of keeping the patch
cops happy, I can submit a patch for this on top of this series.

Regards,

K. Y
 
 --
 
 Sasha.
 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver

2011-07-16 Thread Christoph Hellwig 
On Sat, Jul 16, 2011 at 04:01:39PM +0300, Sasha Levin wrote:
 Is think that what Christoph meant was simplifying it to:
 
   if (dev_is_ide)
   storvsc_get_ide_info(device, target, path);
   
   host_dev-path = device_info.path_id;
   host_dev-target = device_info.target_id;

Indeed.  In addition path is unused and target is only used in the
second dev_is_ide conditional, so both the call and the target and path
variables can be moved into that one to further simplify the function.

But I'm fine with delaying that cleanup to a follow on patch so that
we can make progress in getting this pile merged.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver

2011-07-16 Thread Sasha Levin 
On Sat, 2011-07-16 at 12:57 +, KY Srinivasan wrote:
 
  -Original Message-
  From: Christoph Hellwig [mailto:h...@infradead.org]
  Sent: Friday, July 15, 2011 10:05 PM
  To: KY Srinivasan
  Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
  de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
  Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE 
  devices
  using the storvsc driver
  
  Thanks, this looks much cleaner than the initial variant.
  
   + if (dev_is_ide) {
   + storvsc_get_ide_info(device, target, path);
   + host_dev-path = device_info.path_id;
   + host_dev-target = device_info.target_id;
   + } else {
   + host_dev-path = device_info.path_id;
   + host_dev-target = device_info.target_id;
   + }
  
  Is using the device_info values in both branches intentional?  If so
  there's no need to have these assignments duplicated.
 
 While we set the values in both the branches, the value set is different;
 The IDE side encodes the bits differently and is appropriately parsed in the
 function storvsc_get_ide_info().

Is think that what Christoph meant was simplifying it to:

if (dev_is_ide)
storvsc_get_ide_info(device, target, path);

host_dev-path = device_info.path_id;
host_dev-target = device_info.target_id;

-- 

Sasha.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver

2011-07-15 Thread Christoph Hellwig 
Thanks, this looks much cleaner than the initial variant.

 + if (dev_is_ide) {
 + storvsc_get_ide_info(device, target, path);
 + host_dev-path = device_info.path_id;
 + host_dev-target = device_info.target_id;
 + } else {
 + host_dev-path = device_info.path_id;
 + host_dev-target = device_info.target_id;
 + }

Is using the device_info values in both branches intentional?  If so
there's no need to have these assignments duplicated.

 + if (dev_is_ide) {
 + ret = scsi_add_device(host, 0, target, 0);
 + if (ret) {
 + scsi_remove_host(host);
 + goto err_out;
 + }

I'd add another goto error label for this piece of error handling.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization