* Re: [net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning
From: Jeff Kirsher @ 2012-08-24 21:19 UTC (permalink / raw)
To: Joe Perches, Allan, Bruce W; +Cc: David Miller, netdev, gospo, sassmann
In-Reply-To: <1345831348.32359.8.camel@joe2Laptop>
[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]
On Fri, 2012-08-24 at 11:02 -0700, Joe Perches wrote:
> On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:
> > On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:
> > > From: Bruce Allan <bruce.w.allan@intel.com>
> > >
> > > checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...
> > []
> > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > []
> > > @@ -4330,9 +4330,8 @@ static void e1000_print_link_info(struct e1000_adapter *adapter)
> > > u32 ctrl = er32(CTRL);
> > >
> > > /* Link status message must follow this format for user tools */
> > > - printk(KERN_INFO "e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
> > > - adapter->netdev->name,
> > > - adapter->link_speed,
> > > + pr_info("e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
> > > + adapter->netdev->name, adapter->link_speed,
> > > adapter->link_duplex == FULL_DUPLEX ? "Full" : "Half",
> > > (ctrl & E1000_CTRL_TFCE) && (ctrl & E1000_CTRL_RFCE) ? "Rx/Tx" :
> > > (ctrl & E1000_CTRL_RFCE) ? "Rx" :
> >
> > I think these conversions are not a good idea.
> >
> > When you have a specific message format that must be
> > followed, use printk.
> >
> > pr_<level> may at some point in the near future use
> > #define pr_fmt(fmt) KBUiLD_MODNAME ": " fmt
> > as a global default equivalent.
>
> Hey Jeff.
>
> The comment above this change (and the other) reads
>
> /* Link status message must follow this format for user tools */
>
> This file already uses #define pr_fmt(fmt) KBUILD_MODNAME...
> With this patch, the output form changes to use 2 prefixes.
>
> Is that really desired? Probably not.
>
> If the comments are old and don't apply any more, they
> should be removed.
>
Bruce really should answer this since this is his patch and there was a
reason why he made the change. My guess was the current output was
providing incorrect or mis-leading information.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Nicholas A. Bellinger @ 2012-08-24 21:17 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <5037C997.4030803@chelsio.com>
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>
> >> +#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..
^ permalink raw reply
* Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges
From: Nicolas de Pesloüan @ 2012-08-24 21:09 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: vyasevic, netdev
In-Reply-To: <20120824134410.75827e0d@nehalam.linuxnetplumber.net>
Le 24/08/2012 22:44, Stephen Hemminger a écrit :
<snip>
> The vlan map table would be helpful, but the Spanning Tree implementation
> doesn't have a clue about what it means.
Yes, but a possibly future userland implementation of MSTP (or PVST) would benefit from the ability
to manage per VLAN port states.
So at least we should have this in mind while integrating VLAN into the bridge code.
Nicolas.
^ permalink raw reply
* Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).
From: Nicholas A. Bellinger @ 2012-08-24 20:56 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <5037BC94.1050201@chelsio.com>
On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote:
> On 8/24/2012 1:18 AM, Nicholas A. Bellinger wrote:
> > On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
> >> This patch contains code to implement the interrupt handling and the fast
> >> path I/O functionality. The interrupt handling includes allocation of
> >> MSIX vectors, registering and implemeting the interrupt service routines.
> >> The fast path I/O functionality includes posting the I/O request to firmware
> >> via Work Requests, tracking/completing them, and handling task management
> >> requests. SCSI midlayer host template implementation is also covered by
> >> this patch.
> >>
> >> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
> >> ---
> >
> > Hi Naresh,
> >
> > My review comments are inline below..
>
> Hi Nicholas,
>
> Thanks for taking the time to review the driver. Please find my replies
> inline.
>
> Regards,
> Naresh.
>
> >
> >> drivers/scsi/csiostor/csio_isr.c | 631 ++++++++++
> >> drivers/scsi/csiostor/csio_scsi.c | 2498 +++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 3129 insertions(+), 0 deletions(-)
> >> create mode 100644 drivers/scsi/csiostor/csio_isr.c
> >> create mode 100644 drivers/scsi/csiostor/csio_scsi.c
> >>
<SNIP>
> >
> >> diff --git a/drivers/scsi/csiostor/csio_scsi.c b/drivers/scsi/csiostor/csio_scsi.c
> >> new file mode 100644
> >> index 0000000..0f87b00
> >> --- /dev/null
> >> +++ b/drivers/scsi/csiostor/csio_scsi.c
> >> +
> >> +#define CSIO_SCSI_CMD_WR_SZ(_imm) \
> >> + (sizeof(struct fw_scsi_cmd_wr) + /* WR size */ \
> >> + ALIGN((_imm), 16)) /* Immed data */
> >> +
> >> +#define CSIO_SCSI_CMD_WR_SZ_16(_imm) \
> >> + (ALIGN(CSIO_SCSI_CMD_WR_SZ((_imm)), 16))
> >> +
> >> +/*
> >> + * csio_scsi_cmd - Create a SCSI CMD WR.
> >> + * @req: IO req structure.
> >> + *
> >> + * Gets a WR slot in the ingress queue and initializes it with SCSI CMD WR.
> >> + *
> >> + */
> >> +static inline void
> >> +csio_scsi_cmd(struct csio_ioreq *req)
> >> +{
> >> + struct csio_wr_pair wrp;
> >> + struct csio_hw *hw = req->lnode->hwp;
> >> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
> >> + uint32_t size = CSIO_SCSI_CMD_WR_SZ_16(scsim->proto_cmd_len);
> >> +
> >> + req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
> >> + if (unlikely(req->drv_status != CSIO_SUCCESS))
> >> + return;
> >> +
> >> + if (wrp.size1 >= size) {
> >> + /* Initialize WR in one shot */
> >> + csio_scsi_init_cmd_wr(req, wrp.addr1, size);
> >> + } else {
> >> + uint8_t tmpwr[512];
> >
> > Mmmm, putting this large of a buffer on the local stack is probably not
> > a good idea.
> >
> > This should become an allocation.. If it's a hot path then you'll
> > probably want to set this up before-hand.
> >
>
> The else switch above is entered only when we near the end of the DMA
> ring. This is not a frequent occurrence. If it is a strict no-no to have
> so many on-stack bytes, I have to think of a way re-work it to use
> pre-allocated memory. Please let me know.
>
Considering it's used a number of times, it would be better to just
figure out a sensible manner to pre-allocate this, especially if it's
only a single occurrence to an individual ring held under a lock.
> >> + /*
> >> + * Make a temporary copy of the WR and write back
> >> + * the copy into the WR pair.
> >> + */
> >> + csio_scsi_init_cmd_wr(req, (void *)tmpwr, size);
> >> + memcpy(wrp.addr1, tmpwr, wrp.size1);
> >> + memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
> >> + }
> >> +}
> >> +
> >> +/*
> >> + * The following is fast path code. Therefore it is inlined with multi-line
> >> + * macros using name substitution, thus avoiding if-else switches for
> >> + * operation (read/write), as well as serving the purpose of code re-use.
> >> + */
> >> +/*
> >> + * csio_scsi_init_ulptx_dsgl - Fill in a ULP_TX_SC_DSGL
> >> + * @hw: HW module
> >> + * @req: IO request
> >> + * @sgl: ULP TX SGL pointer.
> >> + *
> >> + */
> >> +#define csio_scsi_init_ultptx_dsgl(hw, req, sgl) \
> >> +do { \
> >> + struct ulptx_sge_pair *_sge_pair = NULL; \
> >> + struct scatterlist *_sgel; \
> >> + uint32_t _i = 0; \
> >> + uint32_t _xfer_len; \
> >> + struct list_head *_tmp; \
> >> + struct csio_dma_buf *_dma_buf; \
> >> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
> >> + \
> >> + (sgl)->cmd_nsge = htonl(ULPTX_CMD(ULP_TX_SC_DSGL) | ULPTX_MORE | \
> >> + ULPTX_NSGE((req)->nsge)); \
> >> + /* Now add the data SGLs */ \
> >> + if (likely(!(req)->dcopy)) { \
> >> + scsi_for_each_sg(scmnd, _sgel, (req)->nsge, _i) { \
> >> + if (_i == 0) { \
> >> + (sgl)->addr0 = cpu_to_be64( \
> >> + sg_dma_address(_sgel)); \
> >> + (sgl)->len0 = cpu_to_be32( \
> >> + sg_dma_len(_sgel)); \
> >> + _sge_pair = \
> >> + (struct ulptx_sge_pair *)((sgl) + 1); \
> >> + continue; \
> >> + } \
> >> + if ((_i - 1) & 0x1) { \
> >> + _sge_pair->addr[1] = cpu_to_be64( \
> >> + sg_dma_address(_sgel)); \
> >> + _sge_pair->len[1] = cpu_to_be32( \
> >> + sg_dma_len(_sgel)); \
> >> + _sge_pair++; \
> >> + } else { \
> >> + _sge_pair->addr[0] = cpu_to_be64( \
> >> + sg_dma_address(_sgel)); \
> >> + _sge_pair->len[0] = cpu_to_be32( \
> >> + sg_dma_len(_sgel)); \
> >> + } \
> >> + } \
> >> + } else { \
> >> + /* Program sg elements with driver's DDP buffer */ \
> >> + _xfer_len = scsi_bufflen(scmnd); \
> >> + list_for_each(_tmp, &(req)->gen_list) { \
> >> + _dma_buf = (struct csio_dma_buf *)_tmp; \
> >> + if (_i == 0) { \
> >> + (sgl)->addr0 = cpu_to_be64(_dma_buf->paddr); \
> >> + (sgl)->len0 = cpu_to_be32( \
> >> + min(_xfer_len, _dma_buf->len)); \
> >> + _sge_pair = \
> >> + (struct ulptx_sge_pair *)((sgl) + 1); \
> >> + } \
> >> + else if ((_i - 1) & 0x1) { \
> >> + _sge_pair->addr[1] = cpu_to_be64( \
> >> + _dma_buf->paddr); \
> >> + _sge_pair->len[1] = cpu_to_be32( \
> >> + min(_xfer_len, _dma_buf->len)); \
> >> + _sge_pair++; \
> >> + } else { \
> >> + _sge_pair->addr[0] = cpu_to_be64( \
> >> + _dma_buf->paddr); \
> >> + _sge_pair->len[0] = cpu_to_be32( \
> >> + min(_xfer_len, _dma_buf->len)); \
> >> + } \
> >> + _xfer_len -= min(_xfer_len, _dma_buf->len); \
> >> + _i++; \
> >> + } \
> >> + } \
> >> +} while (0)
> >> +
> >
> > I don't see any reason why this can't just be a static function..? Why
> > is the macro usage necessary here..?
>
> OK, I will make this static-inline.
>
> >
> >> +/*
> >> + * csio_scsi_init_data_wr - Initialize the READ/WRITE SCSI WR.
> >> + * @req: IO req structure.
> >> + * @oper: read/write
> >> + * @wrp: DMA location to place the payload.
> >> + * @size: Size of WR (including FW WR + immed data + rsp SG entry + data SGL
> >> + * @wrop: _READ_/_WRITE_
> >> + *
> >> + * Wrapper for populating fw_scsi_read_wr/fw_scsi_write_wr.
> >> + */
> >> +#define csio_scsi_init_data_wr(req, oper, wrp, size, wrop) \
> >> +do { \
> >> + struct csio_hw *_hw = (req)->lnode->hwp; \
> >> + struct csio_rnode *_rn = (req)->rnode; \
> >> + struct fw_scsi_##oper##_wr *__wr = (struct fw_scsi_##oper##_wr *)(wrp);\
> >> + struct ulptx_sgl *_sgl; \
> >> + struct csio_dma_buf *_dma_buf; \
> >> + uint8_t _imm = csio_hw_to_scsim(_hw)->proto_cmd_len; \
> >> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
> >> + \
> >> + __wr->op_immdlen = cpu_to_be32(FW_WR_OP(FW_SCSI##wrop##WR) | \
> >> + FW_SCSI##wrop##WR_IMMDLEN(_imm)); \
> >> + __wr->flowid_len16 = cpu_to_be32(FW_WR_FLOWID(_rn->flowid) | \
> >> + FW_WR_LEN16( \
> >> + CSIO_ROUNDUP((size), 16))); \
> >> + __wr->cookie = (uintptr_t) (req); \
> >> + __wr->iqid = (uint16_t)cpu_to_be16(csio_q_physiqid(_hw, \
> >> + (req)->iq_idx));\
> >> + __wr->tmo_val = (uint8_t)((req)->tmo); \
> >> + __wr->use_xfer_cnt = 1; \
> >> + __wr->xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
> >> + __wr->ini_xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
> >> + /* Get RSP DMA buffer */ \
> >> + _dma_buf = &(req)->dma_buf; \
> >> + \
> >> + /* Prepare RSP SGL */ \
> >> + __wr->rsp_dmalen = cpu_to_be32(_dma_buf->len); \
> >> + __wr->rsp_dmaaddr = cpu_to_be64(_dma_buf->paddr); \
> >> + \
> >> + __wr->r4 = 0; \
> >> + \
> >> + __wr->u.fcoe.ctl_pri = 0; \
> >> + __wr->u.fcoe.cp_en_class = 0; \
> >> + __wr->u.fcoe.r3_lo[0] = 0; \
> >> + __wr->u.fcoe.r3_lo[1] = 0; \
> >> + csio_scsi_fcp_cmnd((req), (void *)((uintptr_t)(wrp) + \
> >> + sizeof(struct fw_scsi_##oper##_wr))); \
> >> + \
> >> + /* Move WR pointer past command and immediate data */ \
> >> + _sgl = (struct ulptx_sgl *) ((uintptr_t)(wrp) + \
> >> + sizeof(struct fw_scsi_##oper##_wr) + \
> >> + ALIGN(_imm, 16)); \
> >> + \
> >> + /* Fill in the DSGL */ \
> >> + csio_scsi_init_ultptx_dsgl(_hw, (req), _sgl); \
> >> + \
> >> +} while (0)
> >> +
> >
> > This one has four uses of CPP keys. Just turn those into macros, and
> > leave the rest of the code in a static function.
> >
>
> So what you are suggesting is to have all the lines of the macro
> csio_scsi_init_data_wr() added into a static function, but for the ones
> with the 4 keys. csio_scsi_init_data_wr() will then invoke this new
> function. Is that correct?
>
Not sure how the above should actually look without actually doing it,
but IMHO the usage of macro just obfuscates what is going on..
If it's only used a few times, just inline the code into seperate static
functions. If it's used more than a few times, then use a single static
funciton with macro accessors for the assignment of the various '__wr'
structure members.
The larger problem with all of these macros is that you can't tell what
is a macro and what is a function.
If you need to use a CPP macro, please make sure to capitalize the name
of the macro in order to tell the difference between the two.
> >> +/* Calculate WR size needed for fw_scsi_read_wr/fw_scsi_write_wr */
> >> +#define csio_scsi_data_wrsz(req, oper, sz, imm) \
> >> +do { \
> >> + (sz) = sizeof(struct fw_scsi_##oper##_wr) + /* WR size */ \
> >> + ALIGN((imm), 16) + /* Immed data */ \
> >> + sizeof(struct ulptx_sgl); /* ulptx_sgl */ \
> >> + \
> >> + if (unlikely((req)->nsge > 1)) \
> >> + (sz) += (sizeof(struct ulptx_sge_pair) * \
> >> + (ALIGN(((req)->nsge - 1), 2) / 2)); \
> >> + /* Data SGE */ \
> >> +} while (0)
> >> +
> >> +/*
> >> + * csio_scsi_data - Create a SCSI WRITE/READ WR.
> >> + * @req: IO req structure.
> >> + * @oper: read/write
> >> + * @wrop: _READ_/_WRITE_ (string subsitutions to use with the FW bit field
> >> + * macros).
> >> + *
> >> + * Gets a WR slot in the ingress queue and initializes it with
> >> + * SCSI CMD READ/WRITE WR.
> >> + *
> >> + */
> >> +#define csio_scsi_data(req, oper, wrop) \
> >> +do { \
> >> + struct csio_wr_pair _wrp; \
> >> + uint32_t _size; \
> >> + struct csio_hw *_hw = (req)->lnode->hwp; \
> >> + struct csio_scsim *_scsim = csio_hw_to_scsim(_hw); \
> >> + \
> >> + csio_scsi_data_wrsz((req), oper, _size, _scsim->proto_cmd_len); \
> >> + _size = ALIGN(_size, 16); \
> >> + \
> >> + (req)->drv_status = csio_wr_get(_hw, (req)->eq_idx, _size, &_wrp); \
> >> + if (likely((req)->drv_status == CSIO_SUCCESS)) { \
> >> + if (likely(_wrp.size1 >= _size)) { \
> >> + /* Initialize WR in one shot */ \
> >> + csio_scsi_init_data_wr((req), oper, _wrp.addr1, \
> >> + _size, wrop); \
> >> + } else { \
> >> + uint8_t tmpwr[512]; \
> >> + /* \
> >> + * Make a temporary copy of the WR and write back \
> >> + * the copy into the WR pair. \
> >> + */ \
> >> + csio_scsi_init_data_wr((req), oper, (void *)tmpwr, \
> >> + _size, wrop); \
> >> + memcpy(_wrp.addr1, tmpwr, _wrp.size1); \
> >> + memcpy(_wrp.addr2, tmpwr + _wrp.size1, \
> >> + _size - _wrp.size1); \
> >> + } \
> >> + } \
> >> +} while (0)
> >> +
> >
> > Ditto on this one, along with the tmpwr[512] stack usage..
> >
> >> +static inline void
> >> +csio_scsi_abrt_cls(struct csio_ioreq *req, bool abort)
> >> +{
> >> + struct csio_wr_pair wrp;
> >> + struct csio_hw *hw = req->lnode->hwp;
> >> + uint32_t size = ALIGN(sizeof(struct fw_scsi_abrt_cls_wr), 16);
> >> +
> >> + req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
> >> + if (req->drv_status != CSIO_SUCCESS)
> >> + return;
> >> +
> >> + if (wrp.size1 >= size) {
> >> + /* Initialize WR in one shot */
> >> + csio_scsi_init_abrt_cls_wr(req, wrp.addr1, size, abort);
> >> + } else {
> >> + uint8_t tmpwr[512];
> >
> > Ditto here on local scope stack usage..
> >
> >> + /*
> >> + * Make a temporary copy of the WR and write back
> >> + * the copy into the WR pair.
> >> + */
> >> + csio_scsi_init_abrt_cls_wr(req, (void *)tmpwr, size, abort);
> >> + memcpy(wrp.addr1, tmpwr, wrp.size1);
> >> + memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
> >> + }
> >> +}
> >> +
> >> +/*****************************************************************************/
> >> +/* START: SCSI SM */
> >> +/*****************************************************************************/
> >> +static void
> >> +csio_scsis_uninit(struct csio_ioreq *req, enum csio_scsi_ev evt)
> >> +{
> >> + struct csio_hw *hw = req->lnode->hwp;
> >> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
> >> +
> >> + switch (evt) {
> >> +
> >> + case CSIO_SCSIE_START_IO:
> >
> > Extra space between start of first switch case
>
> OK, I will remove it. Is there any tool that catches such deviations?
> checkpath.pl didnt report it.
>
Sorry, can't help you there.. ;)
> >
> >> +
> >> +/**
> >> + * csio_queuecommand_lck - Entry point to kickstart an I/O request.
> >> + * @cmnd: The I/O request from ML.
> >> + * @done: The ML callback routine.
> >> + *
> >> + * This routine does the following:
> >> + * - Checks for HW and Rnode module readiness.
> >> + * - Gets a free ioreq structure (which is already initialized
> >> + * to uninit during its allocation).
> >> + * - Maps SG elements.
> >> + * - Initializes ioreq members.
> >> + * - Kicks off the SCSI state machine for this IO.
> >> + * - Returns busy status on error.
> >> + */
> >> +static int
> >> +csio_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done)(struct scsi_cmnd *))
> >> +{
> >> + struct csio_lnode *ln = shost_priv(cmnd->device->host);
> >> + struct csio_hw *hw = csio_lnode_to_hw(ln);
> >> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
> >> + struct csio_rnode *rn = (struct csio_rnode *)(cmnd->device->hostdata);
> >> + struct csio_ioreq *ioreq = NULL;
> >> + unsigned long flags;
> >> + int nsge = 0;
> >> + int rv = SCSI_MLQUEUE_HOST_BUSY, nr;
> >> + csio_retval_t retval;
> >> + int cpu;
> >> + struct csio_scsi_qset *sqset;
> >> + struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
> >> +
> >> + if (!blk_rq_cpu_valid(cmnd->request))
> >> + cpu = smp_processor_id();
> >> + else
> >> + cpu = cmnd->request->cpu;
> >> +
> >> + sqset = &hw->sqset[ln->portid][cpu];
> >> +
> >> + nr = fc_remote_port_chkready(rport);
> >> + if (nr) {
> >> + cmnd->result = nr;
> >> + csio_inc_stats(scsim, n_rn_nr_error);
> >> + goto err_done;
> >> + }
> >> +
> >> + if (unlikely(!csio_is_hw_ready(hw))) {
> >> + cmnd->result = (DID_REQUEUE << 16);
> >> + csio_inc_stats(scsim, n_hw_nr_error);
> >> + goto err_done;
> >> + }
> >> +
> >> + /* Get req->nsge, if there are SG elements to be mapped */
> >> + nsge = scsi_dma_map(cmnd);
> >> + if (unlikely(nsge < 0)) {
> >> + csio_inc_stats(scsim, n_dmamap_error);
> >> + goto err;
> >> + }
> >> +
> >> + /* Do we support so many mappings? */
> >> + if (unlikely(nsge > scsim->max_sge)) {
> >> + csio_warn(hw,
> >> + "More SGEs than can be supported."
> >> + " SGEs: %d, Max SGEs: %d\n", nsge, scsim->max_sge);
> >> + csio_inc_stats(scsim, n_unsupp_sge_error);
> >> + goto err_dma_unmap;
> >> + }
> >> +
> >> + /* Get a free ioreq structure - SM is already set to uninit */
> >> + ioreq = csio_get_scsi_ioreq_lock(hw, scsim);
> >> + if (!ioreq) {
> >> + csio_err(hw, "Out of I/O request elements. Active #:%d\n",
> >> + scsim->stats.n_active);
> >> + csio_inc_stats(scsim, n_no_req_error);
> >> + goto err_dma_unmap;
> >> + }
> >> +
> >> + ioreq->nsge = nsge;
> >> + ioreq->lnode = ln;
> >> + ioreq->rnode = rn;
> >> + ioreq->iq_idx = sqset->iq_idx;
> >> + ioreq->eq_idx = sqset->eq_idx;
> >> + ioreq->wr_status = 0;
> >> + ioreq->drv_status = CSIO_SUCCESS;
> >> + csio_scsi_cmnd(ioreq) = (void *)cmnd;
> >> + ioreq->tmo = 0;
> >> +
> >> + switch (cmnd->sc_data_direction) {
> >> + case DMA_BIDIRECTIONAL:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_BIDI;
> >> + csio_inc_stats(ln, n_control_requests);
> >> + break;
> >> + case DMA_TO_DEVICE:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_WRITE;
> >> + csio_inc_stats(ln, n_output_requests);
> >> + ln->stats.n_output_bytes += scsi_bufflen(cmnd);
> >> + break;
> >> + case DMA_FROM_DEVICE:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_READ;
> >> + csio_inc_stats(ln, n_input_requests);
> >> + ln->stats.n_input_bytes += scsi_bufflen(cmnd);
> >> + break;
> >> + case DMA_NONE:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_NONE;
> >> + csio_inc_stats(ln, n_control_requests);
> >> + break;
> >> + default:
> >> + CSIO_DB_ASSERT(0);
> >> + break;
> >> + }
> >> +
> >> + /* Set cbfn */
> >> + ioreq->io_cbfn = csio_scsi_cbfn;
> >> +
> >> + /* Needed during abort */
> >> + cmnd->host_scribble = (unsigned char *)ioreq;
> >> + cmnd->scsi_done = done;
> >> + cmnd->SCp.Message = 0;
> >> +
> >> + /* Kick off SCSI IO SM on the ioreq */
> >> + spin_lock_irqsave(&hw->lock, flags);
> >> + retval = csio_scsi_start_io(ioreq);
> >> + spin_unlock_irqrestore(&hw->lock, flags);
> >> +
> >> + if (retval != CSIO_SUCCESS) {
> >> + csio_err(hw, "ioreq: %p couldnt be started, status:%d\n",
> >> + ioreq, retval);
> >> + csio_inc_stats(scsim, n_busy_error);
> >> + goto err_put_req;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_put_req:
> >> + csio_put_scsi_ioreq_lock(hw, scsim, ioreq);
> >> +err_dma_unmap:
> >> + if (nsge > 0)
> >> + scsi_dma_unmap(cmnd);
> >> +err:
> >> + return rv;
> >> +
> >> +err_done:
> >> + done(cmnd);
> >> + return 0;
> >> +}
> >> +
> >> +static DEF_SCSI_QCMD(csio_queuecommand);
> >> +
> >
> > This means that your running with the host_lock held.. I'm not sure if
> > that is really what you want to do as it really end's up killing
> > multi-lun small packet performance..
> >
> > How about dropping DEF_SCSI_QCMD usage here, and figure out what
> > actually needs to be protected by the SCSI host_lock within
> > csio_queuecommand_lck()..?
>
> It is on my TODO list for the next version of the driver, after the
> initial submission. Per the current design, we shouldnt need the
> host_lock to be held, but I would like to test this change thoroughly
> before I submit it.
>
Sure, it's really quite easy to convert and these days the majority of
high performance LLDs do run in host-lock-less mode.
IIRC the libfc based FCoE initiator driver is doing this too..
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-24 20:53 UTC (permalink / raw)
To: Tejun Heo
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, lw-BthXqXjhjHXQFUHtdCDX3A,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120824203332.GF21325-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On 08/24/2012 10:33 PM, Tejun Heo wrote:
> Hello, Sasha.
>
> On Fri, Aug 24, 2012 at 10:11:55PM +0200, Sasha Levin wrote:
>>> If this implementation is about the common trivial case, why not just
>>> have the usual DECLARE/DEFINE_HASHTABLE() combination?
>>
>> When we add the dynamic non-resizable support, how would DEFINE_HASHTABLE() look?
>
> Hmmm? DECLARE/DEFINE are usually for static ones.
Yup, but we could be using the same API for dynamic non-resizable and static if
we go with the DECLARE/hash_init. We could switch between them (and other
implementations) without having to change the code.
>>> I don't know. If we stick to the static (or even !resize dymaic)
>>> straight-forward hash - and we need something like that - I don't see
>>> what the full encapsulation buys us other than a lot of trivial
>>> wrappers.
>>
>> Which macros do you consider as trivial within the current API?
>>
>> Basically this entire thing could be reduced to DEFINE/DECLARE_HASHTABLE and
>> get_bucket(), but it would make the life of anyone who wants a slightly
>> different hashtable a hell.
>
> Wouldn't the following be enough to get most of the benefits?
>
> * DECLARE/DEFINE
> * hash_head()
> * hash_for_each_head()
> * hash_add*()
> * hash_for_each_possible*()
* hash_for_each*() ?
Why do we need hash_head/hash_for_each_head()? I haven't stumbled on a place yet
that needed direct access to the bucket itself.
Consider the following list:
- DECLARE
- hash_init
- hash_add
- hash_del
- hash_hashed
- hash_for_each_[rcu, safe]
- hash_for_each_possible[rcu, safe]
This basically means 11 macros/functions that would let us have full
encapsulation and will make it very easy for future implementations to work with
this API instead of making up a new one. It's also not significantly (+~2-3)
more than the ones you listed.
>> I think that right now the only real trivial wrapper is hash_hashed(), and I
>> think it's a price worth paying to have a single hashtable API instead of
>> fragmenting it when more implementations come along.
>
> I'm not objecting strongly against full encapsulation but having this
> many thin wrappers makes me scratch my head.
>
> Thanks.
>
^ permalink raw reply
* Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges
From: Stephen Hemminger @ 2012-08-24 20:44 UTC (permalink / raw)
To: vyasevic; +Cc: Nicolas de Pesloüan, netdev
In-Reply-To: <5036EC52.9090406@redhat.com>
On Thu, 23 Aug 2012 22:52:02 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:
> On 08/23/2012 05:03 PM, Nicolas de Pesloüan wrote:
> > Le 23/08/2012 21:29, Vlad Yasevich a écrit :
> >> This series of patches provides an ability to add VLAN IDs to the bridge
> >> ports. This is similar to what can be found in most switches. The
> >> bridge
> >> port may have any number of VLANs added to it including vlan 0 for
> >> untagged
> >> traffic. When vlans are added to the port, only traffic tagged with
> >> particular
> >> vlan will forwarded over this port. Additionally, vlan ids are added
> >> to FDB
> >> entries and become part of the lookup. This way we correctly identify
> >> the FDB
> >> entry.
> >>
> >> There are still pieces missing. I don't yet support adding a static
> >> fdb entry
> >> with a particular vlan. There is no netlink support for carrying a
> >> vlan id.
> >>
> >> I'd like to hear thoughts of whether this is usufull and something we
> >> should
> >> persue.
> >>
> >
> > Do you think this might allow for per VLAN spanning tree (having ports
> > in forwarding state or blocking state depending on the VLAN) in the future?
> >
> > Nicolas.
>
> sure, why not.
The vlan map table would be helpful, but the Spanning Tree implementation
doesn't have a clue about what it means.
^ permalink raw reply
* Re: [linuxppc-release] [PATCH] net/fsl_pq_mdio: add support for the Fman 1G MDIO controller
From: David Miller @ 2012-08-24 20:37 UTC (permalink / raw)
To: timur; +Cc: afleming, netdev
In-Reply-To: <5037E445.9040102@freescale.com>
From: Timur Tabi <timur@freescale.com>
Date: Fri, 24 Aug 2012 15:29:57 -0500
> David Miller wrote:
>>> >
>>> > This patch isn't quite right. I'll post a new version soon.
>
>> I already applied it to net-next, didn't you see?
>
> You applied this patch:
>
> http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commit;h=9f35a7342cff0be72e3c038ea972e07662ca1ce8
Aha, my bad. :)
^ permalink raw reply
* Re: pull request: sfc-next 2012-08-24
From: David Miller @ 2012-08-24 20:36 UTC (permalink / raw)
To: bhutchings; +Cc: linux-net-drivers, netdev
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 24 Aug 2012 20:46:14 +0100
> The following changes since commit 8f4cccbbd92f2ad0ddbbc498ef7cee2a1c3defe9:
>
> net: Set device operstate at registration time (2012-08-24 12:46:13 -0400)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next.git for-davem
>
> (commit 8f8b3d518999fd1c342310910aa1e49112c86d05)
>
> 1. Change the TX path to stop queues earlier and avoid returning
> NETDEV_TX_BUSY.
> 2. Remove some inefficiencies in soft-TSO.
> 3. Fix various bugs involving device state transitions and/or reset
> scheduling by error handlers.
> 4. Take advantage of my previous change to operstate initialisation.
> 5. Miscellaneous cleanup.
Looks good, pulled, thanks Ben.
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Tejun Heo @ 2012-08-24 20:33 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, lw-BthXqXjhjHXQFUHtdCDX3A,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <5037E00B.6090606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hello, Sasha.
On Fri, Aug 24, 2012 at 10:11:55PM +0200, Sasha Levin wrote:
> > If this implementation is about the common trivial case, why not just
> > have the usual DECLARE/DEFINE_HASHTABLE() combination?
>
> When we add the dynamic non-resizable support, how would DEFINE_HASHTABLE() look?
Hmmm? DECLARE/DEFINE are usually for static ones.
> > I don't know. If we stick to the static (or even !resize dymaic)
> > straight-forward hash - and we need something like that - I don't see
> > what the full encapsulation buys us other than a lot of trivial
> > wrappers.
>
> Which macros do you consider as trivial within the current API?
>
> Basically this entire thing could be reduced to DEFINE/DECLARE_HASHTABLE and
> get_bucket(), but it would make the life of anyone who wants a slightly
> different hashtable a hell.
Wouldn't the following be enough to get most of the benefits?
* DECLARE/DEFINE
* hash_head()
* hash_for_each_head()
* hash_add*()
* hash_for_each_possible*()
> I think that right now the only real trivial wrapper is hash_hashed(), and I
> think it's a price worth paying to have a single hashtable API instead of
> fragmenting it when more implementations come along.
I'm not objecting strongly against full encapsulation but having this
many thin wrappers makes me scratch my head.
Thanks.
--
tejun
^ permalink raw reply
* Re: [linuxppc-release] [PATCH] net/fsl_pq_mdio: add support for the Fman 1G MDIO controller
From: Timur Tabi @ 2012-08-24 20:29 UTC (permalink / raw)
To: David Miller; +Cc: afleming, netdev
In-Reply-To: <20120824.162541.256137760046203132.davem@davemloft.net>
David Miller wrote:
>> >
>> > This patch isn't quite right. I'll post a new version soon.
> I already applied it to net-next, didn't you see?
You applied this patch:
http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commit;h=9f35a7342cff0be72e3c038ea972e07662ca1ce8
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [linuxppc-release] [PATCH] net/fsl_pq_mdio: add support for the Fman 1G MDIO controller
From: David Miller @ 2012-08-24 20:25 UTC (permalink / raw)
To: timur; +Cc: afleming, netdev
In-Reply-To: <5037E301.2080805@freescale.com>
From: Timur Tabi <timur@freescale.com>
Date: Fri, 24 Aug 2012 15:24:33 -0500
> Timur Tabi wrote:
>> The MDIO controller on the Frame Manager (Fman) is compatible with the
>> QE and Gianfar MDIO controllers, but we don't care about the TBI because
>> the Ethernet drivers (FMD) take care of programming it.
>>
>> Signed-off-by: Timur Tabi <timur@freescale.com>
>
> This patch isn't quite right. I'll post a new version soon.
I already applied it to net-next, didn't you see?
^ permalink raw reply
* Re: pull request: sfc 2012-08-24
From: David Miller @ 2012-08-24 20:24 UTC (permalink / raw)
To: bhutchings; +Cc: linux-net-drivers, netdev
In-Reply-To: <1345838325.2694.41.camel@bwh-desktop.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 24 Aug 2012 20:58:45 +0100
> Simple fix for a braino. Please also queue this for the 3.4 and 3.5
> stable series.
Ok, pulled and queued up for -stable, thanks Ben.
^ permalink raw reply
* Re: [linuxppc-release] [PATCH] net/fsl_pq_mdio: add support for the Fman 1G MDIO controller
From: Timur Tabi @ 2012-08-24 20:24 UTC (permalink / raw)
To: Andy Fleming, David Miller, netdev
In-Reply-To: <1345757047-14657-1-git-send-email-timur@freescale.com>
Timur Tabi wrote:
> The MDIO controller on the Frame Manager (Fman) is compatible with the
> QE and Gianfar MDIO controllers, but we don't care about the TBI because
> the Ethernet drivers (FMD) take care of programming it.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
This patch isn't quite right. I'll post a new version soon.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-24 20:11 UTC (permalink / raw)
To: Tejun Heo
Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mingo-X9Un+BFzKDI, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, josh-iaAMLnmF4UmaiuxdJuQwMA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
axboe-tSWWG44O7X1aa/9Udqfwiw, agk-H+wXaHxf7aLQT0dZR+AlfA,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, teigland-H+wXaHxf7aLQT0dZR+AlfA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw, fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
jesse-l0M0P4e3n4LQT0dZR+AlfA,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, snitzer-H+wXaHxf7aLQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
dev-yBygre7rU0TnMu66kgdUjQ, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
lw-BthXqXjhjHXQFUHtdCDX3A
In-Reply-To: <20120824195941.GC21325-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On 08/24/2012 09:59 PM, Tejun Heo wrote:
> Hello, Sasha.
>
> On Fri, Aug 24, 2012 at 09:47:19PM +0200, Sasha Levin wrote:
>>> I think this is problematic. It looks exactly like other existing
>>> DEFINE macros yet what its semantics is different. I don't think
>>> that's a good idea.
>>
>> I can switch that to be DECLARE_HASHTABLE() if the issue is semantics.
>
> If this implementation is about the common trivial case, why not just
> have the usual DECLARE/DEFINE_HASHTABLE() combination?
When we add the dynamic non-resizable support, how would DEFINE_HASHTABLE() look?
>>> So, I think it would be best to keep this one as straight-forward and
>>> trivial as possible. Helper macros to help its users are fine but
>>> let's please not go for full encapsulation.
>>
>> What if we cut off the dynamic allocated (but not resizable) hashtable out for
>> the moment, and focus on the most common statically allocated hashtable case?
>>
>> The benefits would be:
>>
>> - Getting rid of all the _size() macros, which will make the amount of helpers
>> here reasonable.
>> - Dynamically allocated hashtable can be easily added as a separate
>> implementation using the same API. We already have some of those in the kernel...
>
> It seems we have enough of this static usage and solving the static
> case first shouldn't hinder the dynamic (!resize) case later, so,
> yeah, sounds good to me.
>
>> - When that's ready, I feel it's a shame to lose full encapsulation just due to
>> hash_hashed().
>
> I don't know. If we stick to the static (or even !resize dymaic)
> straight-forward hash - and we need something like that - I don't see
> what the full encapsulation buys us other than a lot of trivial
> wrappers.
Which macros do you consider as trivial within the current API?
Basically this entire thing could be reduced to DEFINE/DECLARE_HASHTABLE and
get_bucket(), but it would make the life of anyone who wants a slightly
different hashtable a hell.
I think that right now the only real trivial wrapper is hash_hashed(), and I
think it's a price worth paying to have a single hashtable API instead of
fragmenting it when more implementations come along.
Thanks,
Sasha
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] [v3] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Stephen Warren @ 2012-08-24 20:04 UTC (permalink / raw)
To: Timur Tabi
Cc: Andy Fleming, David Miller, ddaney.cavm, devicetree-discuss,
netdev
In-Reply-To: <5037DD60.8090206@freescale.com>
On 08/24/2012 02:00 PM, Timur Tabi wrote:
> Stephen Warren wrote:
>
>>> +This is a special case of a MDIO bus multiplexer. A memory-mapped device,
>>> +like an FPGA, is used to control which child bus is connected. The mdio-mux
>>> +node must be a child of the memory-mapped device. The driver currently only
>>> +supports devices with eight-bit registers.
>>
>> That last sentence seems like a property of the driver, not the binding;
>> I could easily anticipate allowing the size to be 1 or 2 or 4, and a
>> driver adapter to that in the future.
>
> True, but I couldn't think of a better place to mention this. Adding
> support for multi-byte registers also requires handling the endianness of
> those registers. I have that problem with the mdio-mux-gpio driver. That
> driver assumes that the GPIO bits are numbered in little-endian order, so
> my device tree on my big-endian CPU (PowerPC) lists the GPIO pins in
> reverse order.
True. One could always simply assume that the registers are native
endian by default, and then if ever they need not to be, add optional
properties to the binding.
>> Otherwise, this binding looks great now.
>
> Do you still want me to scrub any references to the register size
> requirement from the document?
I don't feel too strongly about it. It seems cleaner to, but not a big deal.
>>> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
>>
>>> +static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child,
>>> + void *data)
>>> +{
>>> + struct mdio_mux_mmioreg_state *s = data;
>>> +
>>> + if (current_child ^ desired_child) {
>>> + void *p = ioremap(s->phys, 1);
>>> + uint8_t x, y;
>>> +
>>> + if (!p)
>>> + return -ENOMEM;
>>
>> Why not map it during probe?
>
> I thought about that, but I generally don't like mappings that exist for
> all eternity even though they're rarely used. Once the interface is up,
> we don't expect any bus muxing to occur.
>
>>
>>> + x = ioread8(p);
>>> + y = (x & ~s->mask) | desired_child;
>>> + if (x != y) {
>>
>> Isn't that always true, given if (current_child ^ desired_child) above?
>
> If current_child == -1, but the bus is already muxed properly, then
> there's no point in setting it. Do you want me to remove the test, or add
> a comment?
Ah right, I suppose that is true. It almost doesn't seem worth writing
the code to ignore that special case, since presumably the register
write is idempotent, but since it's already there you can feel free not
to rip it out!
^ permalink raw reply
* Re: [PATCH] [v3] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Timur Tabi @ 2012-08-24 20:00 UTC (permalink / raw)
To: Stephen Warren
Cc: Andy Fleming, David Miller, ddaney.cavm, devicetree-discuss,
netdev
In-Reply-To: <5037DB43.60704@wwwdotorg.org>
Stephen Warren wrote:
>> +This is a special case of a MDIO bus multiplexer. A memory-mapped device,
>> +like an FPGA, is used to control which child bus is connected. The mdio-mux
>> +node must be a child of the memory-mapped device. The driver currently only
>> +supports devices with eight-bit registers.
>
> That last sentence seems like a property of the driver, not the binding;
> I could easily anticipate allowing the size to be 1 or 2 or 4, and a
> driver adapter to that in the future.
True, but I couldn't think of a better place to mention this. Adding
support for multi-byte registers also requires handling the endianness of
those registers. I have that problem with the mdio-mux-gpio driver. That
driver assumes that the GPIO bits are numbered in little-endian order, so
my device tree on my big-endian CPU (PowerPC) lists the GPIO pins in
reverse order.
> Otherwise, this binding looks great now.
Do you still want me to scrub any references to the register size
requirement from the document?
>> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
>
>> +static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child,
>> + void *data)
>> +{
>> + struct mdio_mux_mmioreg_state *s = data;
>> +
>> + if (current_child ^ desired_child) {
>> + void *p = ioremap(s->phys, 1);
>> + uint8_t x, y;
>> +
>> + if (!p)
>> + return -ENOMEM;
>
> Why not map it during probe?
I thought about that, but I generally don't like mappings that exist for
all eternity even though they're rarely used. Once the interface is up,
we don't expect any bus muxing to occur.
>
>> + x = ioread8(p);
>> + y = (x & ~s->mask) | desired_child;
>> + if (x != y) {
>
> Isn't that always true, given if (current_child ^ desired_child) above?
If current_child == -1, but the bus is already muxed properly, then
there's no point in setting it. Do you want me to remove the test, or add
a comment?
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* [PATCH net] sfc: Fix reporting of IPv4 full filters through ethtool
From: Ben Hutchings @ 2012-08-24 19:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345838325.2694.41.camel@bwh-desktop.uk.solarflarecom.com>
ETHTOOL_GRXCLSRULE returns filters for a TCP/IPv4 or UDP/IPv4 4-tuple
with source and destination swapped.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/ethtool.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 8cba2df..5faedd8 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -863,8 +863,8 @@ static int efx_ethtool_get_class_rule(struct efx_nic *efx,
&ip_entry->ip4dst, &ip_entry->pdst);
if (rc != 0) {
rc = efx_filter_get_ipv4_full(
- &spec, &proto, &ip_entry->ip4src, &ip_entry->psrc,
- &ip_entry->ip4dst, &ip_entry->pdst);
+ &spec, &proto, &ip_entry->ip4dst, &ip_entry->pdst,
+ &ip_entry->ip4src, &ip_entry->psrc);
EFX_WARN_ON_PARANOID(rc);
ip_mask->ip4src = ~0;
ip_mask->psrc = ~0;
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Tejun Heo @ 2012-08-24 19:59 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, lw-BthXqXjhjHXQFUHtdCDX3A,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <5037DA47.9010306-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hello, Sasha.
On Fri, Aug 24, 2012 at 09:47:19PM +0200, Sasha Levin wrote:
> > I think this is problematic. It looks exactly like other existing
> > DEFINE macros yet what its semantics is different. I don't think
> > that's a good idea.
>
> I can switch that to be DECLARE_HASHTABLE() if the issue is semantics.
If this implementation is about the common trivial case, why not just
have the usual DECLARE/DEFINE_HASHTABLE() combination?
> > So, I think it would be best to keep this one as straight-forward and
> > trivial as possible. Helper macros to help its users are fine but
> > let's please not go for full encapsulation.
>
> What if we cut off the dynamic allocated (but not resizable) hashtable out for
> the moment, and focus on the most common statically allocated hashtable case?
>
> The benefits would be:
>
> - Getting rid of all the _size() macros, which will make the amount of helpers
> here reasonable.
> - Dynamically allocated hashtable can be easily added as a separate
> implementation using the same API. We already have some of those in the kernel...
It seems we have enough of this static usage and solving the static
case first shouldn't hinder the dynamic (!resize) case later, so,
yeah, sounds good to me.
> - When that's ready, I feel it's a shame to lose full encapsulation just due to
> hash_hashed().
I don't know. If we stick to the static (or even !resize dymaic)
straight-forward hash - and we need something like that - I don't see
what the full encapsulation buys us other than a lot of trivial
wrappers.
Thanks.
--
tejun
^ permalink raw reply
* pull request: sfc 2012-08-24
From: Ben Hutchings @ 2012-08-24 19:58 UTC (permalink / raw)
To: David Miller; +Cc: linux-net-drivers, netdev
The following changes since commit 78df76a065ae3b5dbcb9a29912adc02f697de498:
ipv4: take rt_uncached_lock only if needed (2012-08-24 11:47:48 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc.git sfc-3.6
(commit ac70b2e9a13423b5efa0178e081936ce6979aea5)
Simple fix for a braino. Please also queue this for the 3.4 and 3.5
stable series.
Ben.
Ben Hutchings (1):
sfc: Fix reporting of IPv4 full filters through ethtool
drivers/net/ethernet/sfc/ethtool.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH net-next 16/16] sfc: Fix the initial device operstate
From: Ben Hutchings @ 2012-08-24 19:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
Following commit 8f4cccb ('net: Set device operstate at registration
time') it is now correct and preferable to set the carrier off before
registering a device.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 92f0020..a606db4 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -2120,6 +2120,9 @@ static int efx_register_netdev(struct efx_nic *efx)
goto fail_locked;
efx_update_name(efx);
+ /* Always start with carrier off; PHY events will detect the link */
+ netif_carrier_off(net_dev);
+
rc = register_netdevice(net_dev);
if (rc)
goto fail_locked;
@@ -2130,9 +2133,6 @@ static int efx_register_netdev(struct efx_nic *efx)
efx_init_tx_queue_core_txq(tx_queue);
}
- /* Always start with carrier off; PHY events will detect the link */
- netif_carrier_off(net_dev);
-
rtnl_unlock();
rc = device_create_file(&efx->pci_dev->dev, &dev_attr_phy_type);
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 15/16] sfc: Assign efx and efx->type as early as possible in efx_pci_probe()
From: Ben Hutchings @ 2012-08-24 19:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
We also stop clearing *efx in efx_init_struct(). This is safe because
alloc_etherdev_mq() already clears it for us.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 4105a666..92f0020 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -2422,13 +2422,12 @@ static const struct efx_phy_operations efx_dummy_phy_operations = {
/* This zeroes out and then fills in the invariants in a struct
* efx_nic (including all sub-structures).
*/
-static int efx_init_struct(struct efx_nic *efx, const struct efx_nic_type *type,
+static int efx_init_struct(struct efx_nic *efx,
struct pci_dev *pci_dev, struct net_device *net_dev)
{
int i;
/* Initialise common structures */
- memset(efx, 0, sizeof(*efx));
spin_lock_init(&efx->biu_lock);
#ifdef CONFIG_SFC_MTD
INIT_LIST_HEAD(&efx->mtd_list);
@@ -2455,8 +2454,6 @@ static int efx_init_struct(struct efx_nic *efx, const struct efx_nic_type *type,
goto fail;
}
- efx->type = type;
-
EFX_BUG_ON_PARANOID(efx->type->phys_addr_channels > EFX_MAX_CHANNELS);
/* Higher numbered interrupt modes are less capable! */
@@ -2660,7 +2657,6 @@ static int efx_pci_probe_main(struct efx_nic *efx)
static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
const struct pci_device_id *entry)
{
- const struct efx_nic_type *type = (const struct efx_nic_type *) entry->driver_data;
struct net_device *net_dev;
struct efx_nic *efx;
int rc;
@@ -2670,10 +2666,12 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
EFX_MAX_RX_QUEUES);
if (!net_dev)
return -ENOMEM;
- net_dev->features |= (type->offload_features | NETIF_F_SG |
+ efx = netdev_priv(net_dev);
+ efx->type = (const struct efx_nic_type *) entry->driver_data;
+ net_dev->features |= (efx->type->offload_features | NETIF_F_SG |
NETIF_F_HIGHDMA | NETIF_F_TSO |
NETIF_F_RXCSUM);
- if (type->offload_features & NETIF_F_V6_CSUM)
+ if (efx->type->offload_features & NETIF_F_V6_CSUM)
net_dev->features |= NETIF_F_TSO6;
/* Mask for features that also apply to VLAN devices */
net_dev->vlan_features |= (NETIF_F_ALL_CSUM | NETIF_F_SG |
@@ -2681,10 +2679,9 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
NETIF_F_RXCSUM);
/* All offloads can be toggled */
net_dev->hw_features = net_dev->features & ~NETIF_F_HIGHDMA;
- efx = netdev_priv(net_dev);
pci_set_drvdata(pci_dev, efx);
SET_NETDEV_DEV(net_dev, &pci_dev->dev);
- rc = efx_init_struct(efx, type, pci_dev, net_dev);
+ rc = efx_init_struct(efx, pci_dev, net_dev);
if (rc)
goto fail1;
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 14/16] sfc: Remove bogus comment about MTU change and RX buffer overrun
From: Ben Hutchings @ 2012-08-24 19:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
RX DMA is limited by the length specified in each descriptor and not
by the MAC. Over-length frames may get into the RX FIFO regardless of
the MAC settings, due to a hardware bug, but they will be truncated by
the packet DMA engine and reported as such in the completion event.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index b29db12..4105a666 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1953,8 +1953,6 @@ static int efx_change_mtu(struct net_device *net_dev, int new_mtu)
netif_dbg(efx, drv, efx->net_dev, "changing MTU to %d\n", new_mtu);
mutex_lock(&efx->mac_lock);
- /* Reconfigure the MAC before enabling the dma queues so that
- * the RX buffers don't overflow */
net_dev->mtu = new_mtu;
efx->type->reconfigure_mac(efx);
mutex_unlock(&efx->mac_lock);
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 13/16] sfc: Remove overly paranoid locking assertions from netdev operations
From: Ben Hutchings @ 2012-08-24 19:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 8 --------
drivers/net/ethernet/sfc/ethtool.c | 2 --
2 files changed, 0 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index d065340..b29db12 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1767,8 +1767,6 @@ static int efx_ioctl(struct net_device *net_dev, struct ifreq *ifr, int cmd)
struct efx_nic *efx = netdev_priv(net_dev);
struct mii_ioctl_data *data = if_mii(ifr);
- EFX_ASSERT_RESET_SERIALISED(efx);
-
/* Convert phy_id from older PRTAD/DEVAD format */
if ((cmd == SIOCGMIIREG || cmd == SIOCSMIIREG) &&
(data->phy_id & 0xfc00) == 0x0400)
@@ -1850,8 +1848,6 @@ static int efx_net_open(struct net_device *net_dev)
struct efx_nic *efx = netdev_priv(net_dev);
int rc;
- EFX_ASSERT_RESET_SERIALISED(efx);
-
netif_dbg(efx, ifup, efx->net_dev, "opening device on CPU %d\n",
raw_smp_processor_id());
@@ -1946,8 +1942,6 @@ static int efx_change_mtu(struct net_device *net_dev, int new_mtu)
struct efx_nic *efx = netdev_priv(net_dev);
int rc;
- EFX_ASSERT_RESET_SERIALISED(efx);
-
rc = efx_check_disabled(efx);
if (rc)
return rc;
@@ -1975,8 +1969,6 @@ static int efx_set_mac_address(struct net_device *net_dev, void *data)
struct sockaddr *addr = data;
char *new_addr = addr->sa_data;
- EFX_ASSERT_RESET_SERIALISED(efx);
-
if (!is_valid_ether_addr(new_addr)) {
netif_err(efx, drv, efx->net_dev,
"invalid ethernet MAC address requested: %pM\n",
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 5d0e2a3..2bd5c2d 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -529,8 +529,6 @@ static void efx_ethtool_self_test(struct net_device *net_dev,
if (!efx_tests)
goto fail;
-
- ASSERT_RTNL();
if (efx->state != STATE_READY) {
rc = -EIO;
goto fail1;
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 12/16] sfc: Fix reset vs probe/remove/PM races involving efx_nic::state
From: Ben Hutchings @ 2012-08-24 19:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
We try to defer resets while the device is not READY, but we're not
doing this quite correctly. In particular, changes to efx_nic::state
are documented as serialised by the RTNL lock, but they aren't.
1. We check whether a reset was requested during probe (suggesting
broken hardware) before we allow requested resets to be scheduled.
This leaves a window where a requested reset would be deferred
indefinitely.
2. Although we cancel the reset work item during device removal,
there are still later operations that can cause it to be scheduled
again. We need to check the state before scheduling it.
3. Since the state can change between scheduling and running of
the work item, we still need to check it there, and we need to
do so *after* acquiring the RTNL lock which serialises state
changes.
4. We must cancel the reset work item during device removal, if the
state could ever have been READY. This wasn't done in some of the
failure paths from efx_pci_probe(). Move the cancellation to
efx_pci_remove_main().
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 84 ++++++++++++++++++++-------------------
1 files changed, 43 insertions(+), 41 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 5555e9f..d065340 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -2112,6 +2112,19 @@ static int efx_register_netdev(struct efx_nic *efx)
rtnl_lock();
+ /* Enable resets to be scheduled and check whether any were
+ * already requested. If so, the NIC is probably hosed so we
+ * abort.
+ */
+ efx->state = STATE_READY;
+ smp_mb(); /* ensure we change state before checking reset_pending */
+ if (efx->reset_pending) {
+ netif_err(efx, probe, efx->net_dev,
+ "aborting probe due to scheduled reset\n");
+ rc = -EIO;
+ goto fail_locked;
+ }
+
rc = dev_alloc_name(net_dev, net_dev->name);
if (rc < 0)
goto fail_locked;
@@ -2141,14 +2154,14 @@ static int efx_register_netdev(struct efx_nic *efx)
return 0;
+fail_registered:
+ rtnl_lock();
+ unregister_netdevice(net_dev);
fail_locked:
+ efx->state = STATE_UNINIT;
rtnl_unlock();
netif_err(efx, drv, efx->net_dev, "could not register net dev\n");
return rc;
-
-fail_registered:
- unregister_netdev(net_dev);
- return rc;
}
static void efx_unregister_netdev(struct efx_nic *efx)
@@ -2171,7 +2184,11 @@ static void efx_unregister_netdev(struct efx_nic *efx)
strlcpy(efx->name, pci_name(efx->pci_dev), sizeof(efx->name));
device_remove_file(&efx->pci_dev->dev, &dev_attr_phy_type);
- unregister_netdev(efx->net_dev);
+
+ rtnl_lock();
+ unregister_netdevice(efx->net_dev);
+ efx->state = STATE_UNINIT;
+ rtnl_unlock();
}
/**************************************************************************
@@ -2309,13 +2326,15 @@ static void efx_reset_work(struct work_struct *data)
if (!pending)
return;
- /* If we're not READY then don't reset. Leave the reset_pending
- * flags set so that efx_pci_probe_main will be retried */
- if (efx->state != STATE_READY)
- return;
-
rtnl_lock();
- (void)efx_reset(efx, fls(pending) - 1);
+
+ /* We checked the state in efx_schedule_reset() but it may
+ * have changed by now. Now that we have the RTNL lock,
+ * it cannot change again.
+ */
+ if (efx->state == STATE_READY)
+ (void)efx_reset(efx, fls(pending) - 1);
+
rtnl_unlock();
}
@@ -2341,6 +2360,13 @@ void efx_schedule_reset(struct efx_nic *efx, enum reset_type type)
}
set_bit(method, &efx->reset_pending);
+ smp_mb(); /* ensure we change reset_pending before checking state */
+
+ /* If we're not READY then just leave the flags set as the cue
+ * to abort probing or reschedule the reset later.
+ */
+ if (ACCESS_ONCE(efx->state) != STATE_READY)
+ return;
/* efx_process_channel() will no longer read events once a
* reset is scheduled. So switch back to poll'd MCDI completions. */
@@ -2485,6 +2511,12 @@ static void efx_fini_struct(struct efx_nic *efx)
*/
static void efx_pci_remove_main(struct efx_nic *efx)
{
+ /* Flush reset_work. It can no longer be scheduled since we
+ * are not READY.
+ */
+ BUG_ON(efx->state == STATE_READY);
+ cancel_work_sync(&efx->reset_work);
+
#ifdef CONFIG_RFS_ACCEL
free_irq_cpu_rmap(efx->net_dev->rx_cpu_rmap);
efx->net_dev->rx_cpu_rmap = NULL;
@@ -2510,11 +2542,8 @@ static void efx_pci_remove(struct pci_dev *pci_dev)
/* Mark the NIC as fini, then stop the interface */
rtnl_lock();
- efx->state = STATE_UNINIT;
dev_close(efx->net_dev);
efx_stop_interrupts(efx, false);
-
- /* Allow any queued efx_resets() to complete */
rtnl_unlock();
efx_sriov_fini(efx);
@@ -2522,12 +2551,6 @@ static void efx_pci_remove(struct pci_dev *pci_dev)
efx_mtd_remove(efx);
- /* Wait for any scheduled resets to complete. No more will be
- * scheduled from this point because efx_stop_all() has been
- * called, we are no longer registered with driverlink, and
- * the net_device's have been removed. */
- cancel_work_sync(&efx->reset_work);
-
efx_pci_remove_main(efx);
efx_fini_io(efx);
@@ -2686,30 +2709,9 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
goto fail2;
rc = efx_pci_probe_main(efx);
-
- /* Serialise against efx_reset(). No more resets will be
- * scheduled since efx_stop_all() has been called, and we have
- * not and never have been registered.
- */
- cancel_work_sync(&efx->reset_work);
-
if (rc)
goto fail3;
- /* If there was a scheduled reset during probe, the NIC is
- * probably hosed anyway.
- */
- if (efx->reset_pending) {
- netif_err(efx, probe, efx->net_dev,
- "aborting probe due to scheduled reset\n");
- rc = -EIO;
- goto fail4;
- }
-
- /* Switch to the READY state before we expose the device to the OS,
- * so that dev_open()|efx_start_all() will actually start the device */
- efx->state = STATE_READY;
-
rc = efx_register_netdev(efx);
if (rc)
goto fail4;
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 11/16] sfc: Improve log messages in case we abort probe due to a pending reset
From: Ben Hutchings @ 2012-08-24 19:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
The current informational message doesn't properly explain what
happens, and could also appear if we defer a reset during
suspend/resume.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 977fc3a..5555e9f 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -2311,11 +2311,8 @@ static void efx_reset_work(struct work_struct *data)
/* If we're not READY then don't reset. Leave the reset_pending
* flags set so that efx_pci_probe_main will be retried */
- if (efx->state != STATE_READY) {
- netif_info(efx, drv, efx->net_dev,
- "scheduled reset quenched; NIC not ready\n");
+ if (efx->state != STATE_READY)
return;
- }
rtnl_lock();
(void)efx_reset(efx, fls(pending) - 1);
@@ -2703,6 +2700,8 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
* probably hosed anyway.
*/
if (efx->reset_pending) {
+ netif_err(efx, probe, efx->net_dev,
+ "aborting probe due to scheduled reset\n");
rc = -EIO;
goto fail4;
}
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox