> -----Original Message----- > From: Christoph Hellwig [mailto:hch@xxxxxxxxxxxxx] > Sent: Thursday, June 30, 2011 3:39 PM > To: KY Srinivasan > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; Haiyang Zhang; > Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen > Subject: Re: [PATCH 23/40] Staging: hv: storvsc: Introduce code to manage IDE > devices using storvsc HBA > > > +/* > > + * We want to manage the IDE devices using standard Linux SCSI drivers > > + * using the storvsc driver. > > + * Define special channels to support this. > > + */ > > + > > +#define HV_MAX_IDE_DEVICES 4 > > +#define HV_IDE_BASE_CHANNEL 10 > > +#define HV_IDE0_DEV1 HV_IDE_BASE_CHANNEL > > +#define HV_IDE0_DEV2 (HV_IDE_BASE_CHANNEL + 1) > > +#define HV_IDE1_DEV1 (HV_IDE_BASE_CHANNEL + 2) > > +#define HV_IDE1_DEV2 (HV_IDE_BASE_CHANNEL + 3) > > This at last needs a good explanation of why these devices are called > IDE if they actually aren't. I know you've explained the reason to me > before, but it should also be in the code. These devices are configured as IDE devices for the guest. The current emulator supports 2 IDE controllers for a total of potentially 4 devices. I did this to support all these 4 devices under one scsi host and used the channel information to get at the correct device in the I/O path. So, if you go to a model with one host per device, this would not be required. > > The HV_IDE1_DEVn defines don't seem to useful to me. They are just > used in one place, and doing an opencoded HV_IDE_BASE_CHANNEL + > channel_nr would seem a lot easier to understand to me. > > > +static struct Scsi_Host *storvsc_host; > > + > > +/* > > + * State to manage IDE devices that register with the storvsc driver. > > + * > > + */ > > +static struct hv_device *ide_devices[HV_MAX_IDE_DEVICES]; > > + > > +static void storvsc_get_ide_info(struct hv_device *dev, int *target, int *path) > > +{ > > + *target = > > + dev->dev_instance.data[5] << 8 | dev->dev_instance.data[4]; > > + > > + *path = > > + dev->dev_instance.data[3] << 24 | dev->dev_instance.data[2] << 16 | > > + dev->dev_instance.data[1] << 8 | dev->dev_instance.data[0]; > > Pretty odd formatting, I'd rather do it as: > > *target = > dev->dev_instance.data[5] << 8 | > dev->dev_instance.data[4]; > > but more importanly what does path actually stand for here? Opencoding > this into the caller and adding proper comments explaining the scheme > might be more readable. In the blkvsc driver, the path/target info was used to properly identify the device - (a) the device was under the first or second IDE controller and (b) whether it is the first or second device under the controller. Regards, K. Y . _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
