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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
> >> ---
> >>  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>

> >> +#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?
> 

Convert all functions aside from CSIO_CANCELLED to use normal negative
return values from include/asm-generic/error[-base].h

For the CSIO_CANCELLED case, propagate this status up to the specific
caller using another method..

> >> +#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.
> 

Using a LLD defined retval where %90 of the items are from errno.h is
code duplication.  Please get rid of this.

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

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>
     }

> >> +#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.
> >

AFIACT csio_deq_from_tail is unused..?

Please remove it..

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


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux