Re: [PATCH v2 2/9] snic:Add interrupt, resource firmware interfaces

["Narsimhulu Musini (nmusini)" <nmusini@cisco.com>]

Hi Hannes,

  Thank you for reviewing patches. Please find responses inline.
I will incorporate the comments and suggestions in next patch submittal.



On 07/04/15 11:57 am, "Hannes Reinecke" <hare@suse.de> wrote:

>Hi Narsimhulu,
>
>On 04/02/2015 09:48 AM, Narsimhulu Musini (nmusini) wrote:
>> Hi Hannes,
>> 
>>   Thank you for reviewing patches. Please find responses inline.
>> I will incorporate the comments and suggestions in next patch submittal.
>> 
>> 
>> 
>> On 25/03/15 3:48 pm, "Hannes Reinecke" <hare@suse.de> wrote:
>> 
>>> On 03/11/2015 06:01 PM, Narsimhulu Musini wrote:
>[ .. ]
>>>> +void
>>>> +snic_free_intr(struct snic *snic)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	/* ONLY interrupt mode MSIX is supported */
>>>> +	for (i = 0; i < ARRAY_SIZE(snic->msix); i++) {
>>>> +		if (snic->msix[i].requested) {
>>>> +			free_irq(snic->msix_entry[i].vector,
>>>> +				 snic->msix[i].devid);
>>>> +		}
>>>> +	}
>>>> +} /* end of snic_free_intr */
>>>> +
>>>> +int
>>>> +snic_request_intr(struct snic *snic)
>>>> +{
>>>> +	int ret = 0, i;
>>>> +
>>>> +#ifdef SNIC_DEBUG
>>>> +	enum vnic_dev_intr_mode intr_mode;
>>>> +
>>>> +	intr_mode = vnic_dev_get_intr_mode(snic->vdev);
>>>> +	SNIC_BUG_ON(intr_mode != VNIC_DEV_INTR_MODE_MSIX);
>>>> +#endif
>>>> +
>>>> +	/* FIXME: Pass devid as work queue or completion queue pointers
>>>> +	 * except for err_notify
>>>> +	 */
>>>> +	sprintf(snic->msix[SNIC_MSIX_WQ].devname,
>>>> +		"%.11s-scsi-wq",
>>>> +		snic->name);
>>> Any reason why you didn't fix it now?
>>> This is a new submission, so I would have expected
>>> _you_ are fine with the code ...
>> The comment is a place holder for future changes when hardware supports
>> multiple queues. one idea is to pass queue pointer and retrieve snic
>>from
>> queue pointer. At this moment, hardware provides single queue. So
>>directly
>> passing snic.
>> 
>> 
>>>
>>>> +	snic->msix[SNIC_MSIX_WQ].isr = snic_isr_msix_wq;
>>>> +	snic->msix[SNIC_MSIX_WQ].devid = snic;
>>>> +
>>>> +	/* FIXME: name can be scsi_cq or iocmpl */
>>>> +	sprintf(snic->msix[SNIC_MSIX_IO_CMPL].devname,
>>>> +		"%.11s-io-cmpl",
>>>> +		snic->name);
>>> Same here.
>>> I would accept FIXMEs is if they require an infrastructure
>>> change or if it refers to future / planned
>>> hardware updates. This doesn't strike me as either ...
>> The comment is a place holder for future changes when hardware supports
>> multiple queues. one idea is to pass queue pointer and retrieve snic
>>from
>> queue pointer. At this moment, hardware provides single queue. So
>>directly
>> passing snic.
>> 
>Okay, can you then please update the comment indicating this?
>It's not directly a 'FIXME', as the current hardware/firmware
>doesn't support this.
>At the same time I see the value of having this comment in there.
Sure, I will update.

>
>Cheers,
>
>Hannes
>-- 
>Dr. Hannes Reinecke		               zSeries & Storage
>hare@suse.de			               +49 911 74053 688
>SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
>GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
>HRB 21284 (AG Nürnberg)
Thanks
Narsimhulu
>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html