Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).

[Naresh Kumar Inna <naresh@chelsio.com>]

On 8/26/2012 12:10 AM, Nicholas A. Bellinger wrote:
> On Sat, 2012-08-25 at 23:39 +0530, Naresh Kumar Inna wrote:
>> On 8/25/2012 2:47 AM, Nicholas A. Bellinger wrote:
>>> On Sat, 2012-08-25 at 00:06 +0530, Naresh Kumar Inna wrote:
>>>> On 8/24/2012 1:28 AM, Nicholas A. Bellinger wrote:
>>>>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
>>>>>> This patch contains the first set of the header files for csiostor driver.
>>>>>>
>>>>>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
>>>>>> ---
>>>>>>  drivers/scsi/csiostor/csio_defs.h       |  143 ++++++
>>>>>>  drivers/scsi/csiostor/csio_fcoe_proto.h |  843 +++++++++++++++++++++++++++++++
>>>>>>  drivers/scsi/csiostor/csio_hw.h         |  668 ++++++++++++++++++++++++
>>>>>>  drivers/scsi/csiostor/csio_init.h       |  158 ++++++
>>>>>>  4 files changed, 1812 insertions(+), 0 deletions(-)
>>>>>>  create mode 100644 drivers/scsi/csiostor/csio_defs.h
>>>>>>  create mode 100644 drivers/scsi/csiostor/csio_fcoe_proto.h
>>>>>>  create mode 100644 drivers/scsi/csiostor/csio_hw.h
>>>>>>  create mode 100644 drivers/scsi/csiostor/csio_init.h
>>>>>>
>>>>>
>>>>> Hi Naresh,
>>>>>
>>>>> Just commenting on csio_defs.h bits here...  As Robert mentioned, you'll
>>>>> need to convert the driver to use (or add to) upstream protocol
>>>>> definitions and drop the csio_fcoe_proto.h bits..
>>>>>
>>>>
>>>> Hi Nicholas,
>>>>
>>>> I would like take up the discussion of the protocol header file in that
>>>> email thread. Please find the rest of my replies inline.
>>>>
>>>> Thanks for reviewing,
>>>> Naresh.
>>>>
>>>>>> diff --git a/drivers/scsi/csiostor/csio_defs.h b/drivers/scsi/csiostor/csio_defs.h
>>>>>> new file mode 100644
>>>>>> index 0000000..4f1c713
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/scsi/csiostor/csio_defs.h
> 
> <SNIP>
> 
>>>>>> +static inline int
>>>>>> +csio_list_deleted(struct list_head *list)
>>>>>> +{
>>>>>> +	return ((list->next == list) && (list->prev == list));
>>>>>> +}
>>>>>> +
>>>>>> +#define csio_list_next(elem)	(((struct list_head *)(elem))->next)
>>>>>> +#define csio_list_prev(elem)	(((struct list_head *)(elem))->prev)
>>>>>> +
>>>>>> +#define csio_deq_from_head(head, elem)					  \
>>>>>> +do {									  \
>>>>>> +	if (!list_empty(head)) {					  \
>>>>>> +		*((struct list_head **)(elem)) = csio_list_next((head));  \
>>>>>> +		csio_list_next((head)) =				  \
>>>>>> +				csio_list_next(csio_list_next((head)));   \
>>>>>> +		csio_list_prev(csio_list_next((head))) = (head);	  \
>>>>>> +		INIT_LIST_HEAD(*((struct list_head **)(elem)));	          \
>>>>>> +	} else								  \
>>>>>> +		*((struct list_head **)(elem)) = (struct list_head *)NULL;\
>>>>>> +} while (0)
>>>>>> +
>>>>>
>>>>> This code is confusing as hell..  Why can't you just use normal list.h
>>>>> macros for this..?  
>>>>
>>>> I have not found an equivalent function in list.h that does the above
>>>> and the following macro. Could you please point me to it? I have seen a
>>>> couple of other drivers define their own macros to achieve what this
>>>> macro does, hence I assumed there isnt a list.h macro that does this.
>>>>
>>>
>>> AFAICT all that csio_deq_from_head code is supposed to do is pull an
>>> item off a list, right..?  Why not just:
>>>
>>>      while (!list_empty(list)) {
>>>             elem = list_first_entry(list, struct elem_type, 
>>>                                     elm_list);
>>>             list_del_init(&elem->elm_list);
>>>
>>>             <do work>
>>>             <free *elem memory>
>>>      }
>>>
>>
>> I will try to come up with a simpler static inline version of the macro.
>> Would that work?
> 
> No.  The point is that the above code is a disaster, and AFAICT there is
> no reason why any of it is necessary to begin with at all.
> 
> Why can't csio_deq_from_head() just become list_first_entry() +
> list_del_init() to do the exact same thing without all of the extra
> overhead of list_head pointer de-reference + assignments..?
> 
> --nab
> 

Yes, that's what I was trying to say.  csio_deq_from_head() will become
a static function comprising list_first_entry + list_del_init(), with
some checks perhaps.

Thanks,
Naresh.