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: Sat, 25 Aug 2012 00:06:07 +0530 [thread overview]
Message-ID: <5037C997.4030803@chelsio.com> (raw)
In-Reply-To: <1345751929.10190.83.camel@haakon2.linux-iscsi.org>
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
>> @@ -0,0 +1,143 @@
>> +/*
>> + * This file is part of the Chelsio FCoE driver for Linux.
>> + *
>> + * Copyright (c) 2008-2012 Chelsio Communications, Inc. All rights reserved.
>> + *
>> + * This software is available to you under a choice of one of two
>> + * licenses. You may choose to be licensed under the terms of the GNU
>> + * General Public License (GPL) Version 2, available from the file
>> + * COPYING in the main directory of this source tree, or the
>> + * OpenIB.org BSD license below:
>> + *
>> + * Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the following
>> + * conditions are met:
>> + *
>> + * - Redistributions of source code must retain the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer.
>> + *
>> + * - Redistributions in binary form must reproduce the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer in the documentation and/or other materials
>> + * provided with the distribution.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#ifndef __CSIO_DEFS_H__
>> +#define __CSIO_DEFS_H__
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/timer.h>
>> +#include <linux/list.h>
>> +#include <linux/bug.h>
>> +#include <linux/pci.h>
>> +#include <linux/jiffies.h>
>> +
>> +/* Function returns */
>> +enum csio_retval {
>> + CSIO_SUCCESS = 0,
>> + CSIO_INVAL = 1,
>> + CSIO_BUSY = 2,
>> + CSIO_NOSUPP = 3,
>> + CSIO_TIMEOUT = 4,
>> + CSIO_NOMEM = 5,
>> + CSIO_NOPERM = 6,
>> + CSIO_RETRY = 7,
>> + CSIO_EPROTO = 8,
>> + CSIO_EIO = 9,
>> + CSIO_CANCELLED = 10,
>> +};
>> +
>
> Please don't assign macros for errno's, and give them positive values.
>
Although some of these return values appear to be mapped to errno
values, there are others that do not have an errno equivalent (example
CSIO_CANCELLED). We may have future needs to have more driver/protocol
specific return values as well. What do you suggest?
>> +#define csio_retval_t enum csio_retval
>
> Please get rid of this csio_retval_t nonsense.
I can get rid of the typedef and use enum csio_retval instead.
>
>> +
>> +enum {
>> + CSIO_FALSE = 0,
>> + CSIO_TRUE = 1,
>> +};
>> +
>
> Same here, please use normal Boolean macros
>
>> +#define CSIO_ROUNDUP(__v, __r) (((__v) + (__r) - 1) / (__r))
>> +#define CSIO_INVALID_IDX 0xFFFFFFFF
>> +#define csio_inc_stats(elem, val) ((elem)->stats.val++)
>> +#define csio_dec_stats(elem, val) ((elem)->stats.val--)
>
> No reason for either of this stats inc+dec macros. Please drop them.
I will get rid of them.
>
>> +#define csio_valid_wwn(__n) ((*__n >> 4) == 0x5 ? CSIO_TRUE : \
>> + CSIO_FALSE)
>> +#define CSIO_WORD_TO_BYTE 4
>> +
>> +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.
>> +#define csio_deq_from_tail(head, elem) \
>> +do { \
>> + if (!list_empty(head)) { \
>> + *((struct list_head **)(elem)) = csio_list_prev((head)); \
>> + csio_list_prev((head)) = \
>> + csio_list_prev(csio_list_prev((head))); \
>> + csio_list_next(csio_list_prev((head))) = (head); \
>> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
>> + } else \
>> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
>> +} while (0)
>> +
>
> Same here.. Please don't use macros like this.
>
>> +/* State machine */
>> +typedef void (*csio_sm_state_t)(void *, uint32_t);
>> +
>> +struct csio_sm {
>> + struct list_head sm_list;
>> + csio_sm_state_t sm_state;
>> +};
>> +
>> +#define csio_init_state(__smp, __state) \
>> + (((struct csio_sm *)(__smp))->sm_state = (csio_sm_state_t)(__state))
>> +
>> +#define csio_set_state(__smp, __state) \
>> + (((struct csio_sm *)(__smp))->sm_state = (csio_sm_state_t)(__state))
>> +
>> +
>> +#define csio_post_event(__smp, __evt) \
>> + (((struct csio_sm *)(__smp))->sm_state((__smp), (uint32_t)(__evt)))
>> +
>> +#define csio_get_state(__smp) (((struct csio_sm *)(__smp))->sm_state)
>> +
>> +#define csio_match_state(__smp, __state) \
>> + (csio_get_state((__smp)) == (csio_sm_state_t)(__state))
>> +
>
> Why does any of the sm_state stuff need to be in macros..? Please
> inline all of this code.
I will change them to static inline.
>
>> +#define CSIO_ASSERT(cond) \
>> +do { \
>> + if (unlikely(!((cond)))) \
>> + BUG(); \
>> +} while (0)
>> +
>> +#ifdef __CSIO_DEBUG__
>> +#define CSIO_DB_ASSERT(__c) CSIO_ASSERT((__c))
>> +#else
>> +#define CSIO_DB_ASSERT(__c)
>> +#endif
>> +
>> +#endif /* ifndef __CSIO_DEFS_H__ */
>
>
next prev parent reply other threads:[~2012-08-24 18:36 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 [this message]
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
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=5037C997.4030803@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