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.
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)
--
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
