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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx +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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
