From: Naresh Kumar Inna <naresh@chelsio•com>
To: "Nicholas A. Bellinger" <nab@linux-iscsi•org>
Cc: "JBottomley@parallels•com" <JBottomley@parallels•com>,
"linux-scsi@vger•kernel.org" <linux-scsi@vger•kernel.org>,
Dimitrios Michailidis <dm@chelsio•com>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
Chethan Seshadri <chethan@chelsio•com>
Subject: Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
Date: Sun, 26 Aug 2012 00:31:05 +0530 [thread overview]
Message-ID: <503920F1.2050801@chelsio.com> (raw)
In-Reply-To: <1345920029.28432.102.camel@haakon2.linux-iscsi.org>
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.
next prev parent reply other threads:[~2012-08-25 19:01 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-23 22:27 [PATCH 0/8] csiostor: Chelsio FCoE offload driver submission Naresh Kumar Inna
2012-08-23 22:27 ` [PATCH 1/8] csiostor: Chelsio FCoE offload driver submission (sources part 1) Naresh Kumar Inna
2012-08-23 22:27 ` [PATCH 2/8] csiostor: Chelsio FCoE offload driver submission (sources part 2) Naresh Kumar Inna
2012-08-23 22:27 ` [PATCH 3/8] csiostor: Chelsio FCoE offload driver submission (sources part 3) Naresh Kumar Inna
2012-08-23 22:27 ` [PATCH 4/8] csiostor: Chelsio FCoE offload driver submission (sources part 4) Naresh Kumar Inna
2012-08-23 22:27 ` [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5) Naresh Kumar Inna
2012-08-23 19:48 ` Nicholas A. Bellinger
2012-08-24 17:40 ` Naresh Kumar Inna
2012-08-24 18:27 ` Joe Perches
2012-08-24 19:07 ` Naresh Kumar Inna
2012-08-24 20:56 ` Nicholas A. Bellinger
2012-08-25 18:36 ` Naresh Kumar Inna
2012-08-25 18:43 ` Nicholas A. Bellinger
2012-08-29 7:47 ` Naresh Kumar Inna
2012-08-23 22:27 ` [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1) Naresh Kumar Inna
2012-08-23 18:15 ` Love, Robert W
2012-08-24 18:45 ` Naresh Kumar Inna
2012-08-23 19:58 ` Nicholas A. Bellinger
2012-08-24 18:36 ` Naresh Kumar Inna
2012-08-24 21:17 ` Nicholas A. Bellinger
2012-08-25 18:09 ` Naresh Kumar Inna
2012-08-25 18:40 ` Nicholas A. Bellinger
2012-08-25 19:01 ` Naresh Kumar Inna [this message]
2012-08-23 22:27 ` [PATCH 7/8] csiostor: Chelsio FCoE offload driver submission (headers part 2) Naresh Kumar Inna
2012-08-23 22:27 ` [PATCH 8/8] cxgb4: Chelsio FCoE offload driver submission (cxgb4 common header updates) Naresh Kumar Inna
2012-08-24 17:04 ` [PATCH 0/8] csiostor: Chelsio FCoE offload driver submission David Miller
2012-08-24 19:04 ` Naresh Kumar Inna
2012-08-24 19:10 ` David Miller
2012-08-25 19:08 ` Naresh Kumar Inna
2012-09-04 5:13 ` Naresh Kumar Inna
2012-09-04 5:34 ` David Miller
2012-08-24 21:45 ` Paul Gortmaker
2012-08-24 21:58 ` James Bottomley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=503920F1.2050801@chelsio.com \
--to=naresh@chelsio$(echo .)com \
--cc=JBottomley@parallels$(echo .)com \
--cc=chethan@chelsio$(echo .)com \
--cc=dm@chelsio$(echo .)com \
--cc=linux-scsi@vger$(echo .)kernel.org \
--cc=nab@linux-iscsi$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox