Re: [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
On Friday 22 August 2008 12:48:44 pm Andrew Morton wrote: > On Fri, 22 Aug 2008 11:40:56 -0700 > > Karen Xie <[EMAIL PROTECTED]> wrote: > > [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver > > > > From: Karen Xie <[EMAIL PROTECTED]> > > > > cxgb3i iSCSI driver. > > > > Signed-off-by: Karen Xie <[EMAIL PROTECTED]> > > I'm going to suggest that this not be merged in this form due to the > __GFP_NOFAIL issues identified below. > Hi Andrew, thanks for the review. We've started fixing the obvious and looking at alternatives for the nofail allocations. Cheers, Divy --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
On Fri, 22 Aug 2008 11:40:56 -0700
Karen Xie <[EMAIL PROTECTED]> wrote:
> [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
>
> From: Karen Xie <[EMAIL PROTECTED]>
>
> cxgb3i iSCSI driver.
>
> Signed-off-by: Karen Xie <[EMAIL PROTECTED]>
I'm going to suggest that this not be merged in this form due to the
__GFP_NOFAIL issues identified below.
>
> drivers/scsi/Kconfig |2
> drivers/scsi/Makefile|1
> drivers/scsi/cxgb3i/Kconfig |6
> drivers/scsi/cxgb3i/Makefile |5
> drivers/scsi/cxgb3i/cxgb3i.h | 155 +++
> drivers/scsi/cxgb3i/cxgb3i_init.c| 109 ++
> drivers/scsi/cxgb3i/cxgb3i_iscsi.c | 824 ++
> drivers/scsi/cxgb3i/cxgb3i_offload.c | 1985
> ++
> drivers/scsi/cxgb3i/cxgb3i_offload.h | 252
> drivers/scsi/cxgb3i/cxgb3i_ulp2.c| 692
> drivers/scsi/cxgb3i/cxgb3i_ulp2.h| 106 ++
> 11 files changed, 4137 insertions(+), 0 deletions(-)
> create mode 100644 drivers/scsi/cxgb3i/Kconfig
> create mode 100644 drivers/scsi/cxgb3i/Makefile
> create mode 100644 drivers/scsi/cxgb3i/cxgb3i.h
> create mode 100644 drivers/scsi/cxgb3i/cxgb3i_init.c
> create mode 100644 drivers/scsi/cxgb3i/cxgb3i_iscsi.c
> create mode 100644 drivers/scsi/cxgb3i/cxgb3i_offload.c
> create mode 100644 drivers/scsi/cxgb3i/cxgb3i_offload.h
> create mode 100644 drivers/scsi/cxgb3i/cxgb3i_ulp2.c
> create mode 100644 drivers/scsi/cxgb3i/cxgb3i_ulp2.h
>
Please use scripts/checkpatch.pl. It finds things which should be fixed.
>
> ...
>
> +/**
> + * struct cxgb3i_tag_format - cxgb3i ulp tag for steering pdu payload
> + *
> + * @rsvd_bits: # of bits used by h/w
> + * @rsvd_shift: shift left
> + * @rsvd_mask: bit mask
> + *
> + */
> +struct cxgb3i_tag_format {
> + unsigned char idx_bits;
> + unsigned char age_bits;
> + unsigned char rsvd_bits;
> + unsigned char rsvd_shift;
> + u32 rsvd_mask;
> +};
Only three of the five fields were documented.
> +/**
> + * struct cxgb3i_ddp_info - cxgb3i direct data placement for pdu payload
> + *
> + * @llimit: lower bound of the page pod memory
> + * @ulimit: upper bound of the page pod memory
> + * @nppods: # of page pod entries
> + * @idx_last:page pod entry last used
> + * @map_lock:lock to synchonize access to the page pod map
> + * @map: page pod map
> + */
> +struct cxgb3i_ddp_info {
> + unsigned int llimit;
> + unsigned int ulimit;
> + unsigned int nppods;
> + unsigned int idx_last;
> + spinlock_t map_lock;
> + u8 *map;
> +};
> +
> +struct cxgb3i_hba {
> + struct cxgb3i_adapter *snic;
> + struct net_device *ndev;
> + struct Scsi_Host *shost;
> +
> + rwlock_t cconn_rwlock;
> + struct list_head cconn_list;
> +};
> +
> +struct cxgb3i_adapter {
> + struct list_head list_head;
> + spinlock_t lock;
> + struct t3cdev *tdev;
> + struct pci_dev *pdev;
> + unsigned char hba_cnt;
> + struct cxgb3i_hba *hba[MAX_NPORTS];
> +
> + unsigned int tx_max_size;
> + unsigned int rx_max_size;
> +
> + struct cxgb3i_tag_format tag_format;
> + struct cxgb3i_ddp_info ddp;
> +};
> +
> +struct cxgb3i_conn {
> + struct list_head list_head;
> +
> + struct cxgb3i_endpoint *cep;
> + struct iscsi_conn *conn;
> + struct cxgb3i_hba *hba;
> +};
> +
> +struct cxgb3i_endpoint {
> + struct s3_conn *c3cn;
> + struct cxgb3i_hba *hba;
> + struct cxgb3i_conn *cconn;
> +};
We got bored of documenting, I see ;)
It's a shame, because documenting the data structures is very useful.
More important than documenting code flow, for example.
>
> ...
>
> +static inline void cxgb3i_parse_tag(struct cxgb3i_tag_format *format,
> + u32 tag, u32 *rsvd_bits, u32 *sw_bits)
> +{
> + if (rsvd_bits)
> + *rsvd_bits = (tag >> format->rsvd_shift) & format->rsvd_mask;
> + if (sw_bits) {
> + *sw_bits = (tag >> (format->rsvd_shift + format->rsvd_bits))
> + << format->rsvd_shift;
> + *sw_bits |= tag & ((1 << format->rsvd_shift) - 1);
> + }
> +}
Far too large to inline, has only one call site. Can be made static
non-inline in cxgb3i_iscsi.c.
> +int cxgb3i_conn_ulp2_xmit(struct iscsi_conn *);
> +
> +void cxgb3i_display_byte_string(char *, unsigned char *, int, int);
> +
> +#endif
> diff --git a/drivers/scsi/cxgb3i/cxgb3i_init.c
> b/drivers/scsi/cxgb3i/cxgb3i_init.c
> new file mode 100644
> index 000..1c91bb0
> --- /dev/null
> +++ b/drivers/scsi/cxgb3i/cxgb3i_init.c
> @@ -0,0 +1,109 @@
> +/* cxgb3i_init.c: Chelsio S3xx iSCSI driver.
> + *
> + * Copyright (c) 2008 Chelsio Communications, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation.
> + *
> + * Written by: Karen Xie ([EMAIL PROTECTED])
>
Re: [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
From: <[EMAIL PROTECTED]> Date: Sat, 23 Aug 2008 14:37:53 +0530 > Exactly. And I am also suggesting that the driver version is not > standard among different vendors. It should not be standardized because every driver maintainer works differently, and every driver is developed differently, and therefore has different needs and desires for the version number. All that matters is that the driver version makes sense to the person maintaining the driver, and works for them when reviewing bug reports. Look, what you're suggesting is to change existing practice and that doesn't belong in the discussion of the review of a specific driver. If you want to bring that up as a topic and change globally how that is handled, bring that up as a seperate topic on linux-kernel. Don't bug the poor driver submitter who is just following existing and accepted practice. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
From: <[EMAIL PROTECTED]> Date: Sat, 23 Aug 2008 13:52:26 +0530 > >I'd suggest that the version number just be removed. It becomes > meaningless (and often misleading) once a driver is in the mainline > kernel. People will >update the driver without changing the version > number. Code external to the driver but which affects it can change. I totally disagree. I find it very useful when I get a debugging dump from the user and they have no idea where their kernel came from nor can figure out how to determine the kernel version. Sure it might sometimes not get updated for trivial patches that bypass the maintainer, but the maintainer is always going to bump it after non-trivial changes. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
RE: [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
>Look, what you're suggesting is to change existing practice and that doesn't belong in the discussion of the review of a specific driver. >If you want to bring that up as a topic and change globally how that is handled, bring that up as a seperate topic on linux-kernel. Sounds reasonable. >Don't bug the poor driver submitter who is just following existing and accepted practice. That wasn't the intention here. It rose out of the discussion that was being suggested that DRV_MODULE_VERSION be removed and I don't think should be done. So, I suggested a followup suggestion from there. Will take that up as you suggested. -Shyam --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
RE: [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
> David Miller wrote: > > Andrew Morton wrote: > >I'd suggest that the version number just be removed. It becomes > meaningless (and often misleading) once a driver is in the mainline > kernel. People will >update the driver without changing the version > number. Code external to the driver but which affects it can change. >I totally disagree. I find it very useful when I get a debugging dump from the user and they have no idea where their kernel came from nor can figure >out how to determine the kernel version. >Sure it might sometimes not get updated for trivial patches that bypass the maintainer, but the maintainer is always going to bump it after non-trivial >changes. Exactly. And I am also suggesting that the driver version is not standard among different vendors. So, why not get them generated in an automatic build process. Something like "kernel-version.driver-version". I am just imagining here. The details can be worked out. -Shyam --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
RE: [PATCH 4/4 2.6.28] cxgb3i - cxgb3i iscsi driver
>Andrew Morton wrote: > > + * > > + * Written by: Karen Xie ([EMAIL PROTECTED]) */ > > + > > +#include "cxgb3i.h" > > + > > +#define DRV_MODULE_NAME "cxgb3i" > > +#define DRV_MODULE_VERSION "1.0.0" >I'd suggest that the version number just be removed. It becomes meaningless (and often misleading) once a driver is in the mainline kernel. People will >update the driver without changing the version number. Code external to the driver but which affects it can change. >The kernel version identifier is really the only way in whcih you and your support people can reproduce a user's code. > > +#define DRV_MODULE_RELDATE "May 1, 2008" >Ditto. It gives us a stick to ask vendors to maintain upstream versions of driver code in the distros. While we are at this. I believe that there is not much of a standard for driver versioning. If we automatically get a driver version from the kernel version then it solves both problems. Thoughts ?? Thanks, Shyam --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
