* Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Roland Dreier @ 2012-03-21 16:42 UTC (permalink / raw)
To: parav.pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5abe3043-81f8-448a-9e55-b29e23f4eb9a-nbYkmrCdWxmgMrCBcu8zE0EOCMrvLtNR@public.gmane.org>
> +struct ib_pd *ocrdma_alloc_pd(struct ib_device *ibdev,
> + struct ib_ucontext *context,
> + struct ib_udata *udata)
> +{
> + struct ocrdma_dev *dev = get_ocrdma_dev(ibdev);
> + struct ocrdma_pd *pd;
> + int status;
> +
> + pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return ERR_PTR(-ENOMEM);
> + pd->dev = dev;
> + if (udata && context) {
> + pd->dpp_enabled = (dev->nic_info.dev_family ==
> + OCRDMA_GEN2_FAMILY) ? true : false;
Writing
(<bool expr>) ? true : false
is pretty silly, since it's just an obfuscated way of writing
<bool expr>
IOW, you can just write
pd->dpp_enabled = (dev->nic_info.dev_family == OCRDMA_GEN2_FAMILY);
> +int ocrdma_dealloc_pd(struct ib_pd *ibpd)
> +{
> + struct ocrdma_pd *pd = get_ocrdma_pd(ibpd);
> + struct ocrdma_dev *dev = pd->dev;
> + int status;
> + u64 usr_db;
> +
> + if (atomic_read(&pd->use_cnt)) {
> + ocrdma_err("%s(%d) pd=0x%x is in use.\n",
> + __func__, dev->id, pd->id);
> + status = -EFAULT;
> + goto dealloc_err;
> + }
all of the use_cnt tracking in this driver seems to duplicate what the rdma
midlayer already does... is there any reason we need that in the low-level
hardware driver too, or can we just get rid of the various use_cnt members?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Roland Dreier @ 2012-03-21 16:34 UTC (permalink / raw)
To: parav.pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <a5e59a7c-d6ff-4c78-89a7-fad7492260b0-nbYkmrCdWxmgMrCBcu8zE0EOCMrvLtNR@public.gmane.org>
> +int ocrdma_qp_state_machine(struct ocrdma_qp *qp, enum ib_qp_state new_ib_state,
> + enum ib_qp_state *old_ib_state)
> +{
> + unsigned long flags;
> + int status = 0;
> + enum ocrdma_qp_state new_state;
> + new_state = get_ocrdma_qp_state(new_ib_state);
> +
> + /* sync with wqe and rqe posting */
> + spin_lock_irqsave(&qp->q_lock, flags);
> +
> + if (old_ib_state)
> + *old_ib_state = get_ibqp_state(qp->state);
> + if (new_state == qp->state) {
> + spin_unlock_irqrestore(&qp->q_lock, flags);
> + return 1;
> + }
> +
> + switch (qp->state) {
> + case OCRDMA_QPS_RST:
> + switch (new_state) {
> + case OCRDMA_QPS_RST:
> + case OCRDMA_QPS_INIT:
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + break;
> + case OCRDMA_QPS_INIT:
> + /* qps: INIT->XXX */
> + switch (new_state) {
> + case OCRDMA_QPS_INIT:
> + case OCRDMA_QPS_RTR:
> + break;
> + case OCRDMA_QPS_ERR:
> + ocrdma_flush_qp(qp);
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + break;
> + case OCRDMA_QPS_RTR:
> + /* qps: RTS->XXX */
> + switch (new_state) {
> + case OCRDMA_QPS_RTS:
> + break;
> + case OCRDMA_QPS_ERR:
> + ocrdma_flush_qp(qp);
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + break;
> + case OCRDMA_QPS_RTS:
> + /* qps: RTS->XXX */
> + switch (new_state) {
> + case OCRDMA_QPS_SQD:
> + case OCRDMA_QPS_SQE:
> + break;
> + case OCRDMA_QPS_ERR:
> + ocrdma_flush_qp(qp);
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + break;
> + case OCRDMA_QPS_SQD:
> + /* qps: SQD->XXX */
> + switch (new_state) {
> + case OCRDMA_QPS_RTS:
> + case OCRDMA_QPS_SQE:
> + case OCRDMA_QPS_ERR:
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + break;
> + case OCRDMA_QPS_SQE:
> + switch (new_state) {
> + case OCRDMA_QPS_RTS:
> + case OCRDMA_QPS_ERR:
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + break;
> + case OCRDMA_QPS_ERR:
> + /* qps: ERR->XXX */
> + switch (new_state) {
> + case OCRDMA_QPS_RST:
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + break;
> + default:
> + status = -EINVAL;
> + break;
> + };
> + if (!status)
> + qp->state = new_state;
> +
> + spin_unlock_irqrestore(&qp->q_lock, flags);
> + return status;
> +}
The switch statement here seems to largely reimpliment ib_modify_qp_is_ok()
(which is exported from the rdma midlayer). Is there some reason that doesn't
work for your driver? I'd rather fix / generalize the core helper
function instead
of having something mostly duplicate in a hardware driver.
^ permalink raw reply
* RE: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: David Laight @ 2012-03-21 16:33 UTC (permalink / raw)
To: Roland Dreier, parav.pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAL1RGDWnc478=ToFQEC51Usn6LLZgLen=CymFZ0C0GnRp9BAsw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
> > +#define is_cqe_wr_imm(cqe) \
> > + ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_WRITE_IMM) ? 1 : 0)
>
> ...similar comment about using readable typesafe inline functions
> instead of macros...
and if you are using #defines, you need to enclose every reference
to the parameters in ().
David
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: David Laight @ 2012-03-21 16:31 UTC (permalink / raw)
To: Roland Dreier, parav.pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAL1RGDVxCE--P78bk0Me5o+ekSzgBYG0UJT6y3O7cK3mUGBjuQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
> > - Header file for userspace library and kernel driver interface.
>
> > +struct ocrdma_alloc_ucontext_resp {
> > + u32 dev_id;
> > + u32 wqe_size;
> > + u32 max_inline_data;
> > + u32 dpp_wqe_size;
> > + u64 ah_tbl_page;
> > + u32 ah_tbl_len;
> > + u32 rsvd;
> > + u8 fw_ver[32];
> > + u32 rqe_size;
> > + u64 rsvd1;
> > +} __packed;
>
> If I'm reading this correctly, you have the 8-byte rsvd1 member at an
> offset only aligned to 4 bytes, because of the __packed directive. It
> would be much better to have these structures laid out so they are
> naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
> __packed directive, which wrecks gcc code generation in some cases.
>
gcc also supports defining types that have non-standard alignment
constraints that can be used to force the same alignment for
64bit fields between i386 and amd64.
Probably __attribute__((aligned,n)) or similar.
This can be used to force 32bit alignment in amd64 code in order
to match definitions in 32bit userspace.
For new things it would make sense to force 64bit alignment
of 64bit fields for 32bit code.
Adding __packed (rather than 32bit alignment) forces the compiler
to generate byte by byte accesses for all the fields on systems
that can't do misaligned accesses in hardware (eg sparc).
David
David
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Roland Dreier @ 2012-03-21 16:26 UTC (permalink / raw)
To: parav.pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <339d9e05-38fd-45b1-83ed-f06277bd1326-nbYkmrCdWxmgMrCBcu8zE0EOCMrvLtNR@public.gmane.org>
> +/* mailbox cmd response */
> +struct ocrdma_mbx_rsp {
> + u32 subsys_op;
> + u32 status;
> + u32 rsp_len;
> + u32 add_rsp_len;
> +} __packed;
...similar comments about only using __packed where you really need it...
> +#define is_cqe_valid(cq, cqe) \
> + (((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_VALID)\
> + == cq->phase) ? 1 : 0)
> +#define is_cqe_for_sq(cqe) \
> + ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ? 0 : 1)
> +#define is_cqe_for_rq(cqe) \
> + ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ? 1 : 0)
> +#define is_cqe_invalidated(cqe) \
> + ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_INVALIDATE) ? \
> + 1 : 0)
> +#define is_cqe_imm(cqe) \
> + ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_IMM) ? 1 : 0)
> +#define is_cqe_wr_imm(cqe) \
> + ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_WRITE_IMM) ? 1 : 0)
...similar comment about using readable typesafe inline functions
instead of macros...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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: use-after-free in usbnet
From: Ming Lei @ 2012-03-21 16:22 UTC (permalink / raw)
To: Alan Stern
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team, Dave Jones
In-Reply-To: <Pine.LNX.4.44L0.1203211201560.1369-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
On Thu, Mar 22, 2012 at 12:12 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Wed, 21 Mar 2012, Ming Lei wrote:
>
>> On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLg@public.gmane.orgedu> wrote:
>> > On Wed, 21 Mar 2012, Ming Lei wrote:
>> >
>> >> Looks it is a general issue about usb_hcd_unlink_urb.
>> >>
>> >> Alan, how about increasing URB reference count before calling unlink1
>> >> inside usb_hcd_unlink_urb to fix this kind of problem?
>> >
>> > No, that won't fix the problem. The URB could complete and be
>> > deallocated even before usb_hcd_unlink_urb() is called, so nothing that
>> > function can do will prevent the error.
>>
>> IMO, driver should not call usb_hcd_unlink_urb after urb is freed from
>> the driver,
>> but this problem is that URB may be freed during usb_hcd_unlink_urb.
>
> Drivers don't call usb_hcd_unlink_urb; they call usb_unlink_urb. This
> sort of thing can happen:
>
> Driver Interrupt handler
> ------ -----------------
> call usb_unlink_urb
> URB completion interrupt occurs
> call usb_hcd_giveback_urb
> completion routine calls usb_free_urb
> URB is deallocated
> call usb_hcd_unlink_urb
> try to increment URB's refcount
> oops because URB is gone
Got it, thanks for your detailed explanation.
>> In fact, it is allowed that usb_free_urb is called inside .complete handler,
>> at least as said in Documentation/URB.txt:
>>
>> "You may free an urb that you've submitted,..."
>>
>> So looks reasonable to increase the URB reference count before calling
>> unlink1(), just like that done inside usb_hcd_flush_endpoint(). And I
>> think it is a general solution for avoiding this kind of problem.
>
> It will not solve the problem illustrated above. The driver must avoid
> freeing the URB before usb_unlink_urb returns. In this case,
> increasing the refcount around the unlink call would work.
Yes, it is the right fix.
>> > It is the caller's responsibility to make sure that the URB does not
>> > get freed before usb_unlink_urb() or usb_kill_urb() returns. We
>> > probably should mention this in the kerneldoc...
>>
>> If so, looks it is a bit contrary with Documentation/URB.txt, also
>> this may add extra constraint(maybe unnecessary) to the driver.
>
> It's not contradictory. You may indeed free an URB that you have
> submitted, so long as you don't free it while usb_unlink_urb (or
> related routines like usb_kill_urb) is running.
Maybe it should be documented.
So looks the correct fix should be below:
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 4b8b52c..e36a821 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -588,7 +588,7 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
entry = (struct skb_data *) skb->cb;
urb = entry->urb;
-
+ usb_get_urb(urb);
spin_unlock_irqrestore(&q->lock, flags);
// during some PM-driven resume scenarios,
// these (async) unlinks complete immediately
@@ -597,6 +597,7 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
else
count++;
+ usb_put_urb(urb);
spin_lock_irqsave(&q->lock, flags);
}
spin_unlock_irqrestore (&q->lock, flags);
@@ -1028,7 +1029,6 @@ static void tx_complete (struct urb *urb)
}
usb_autopm_put_interface_async(dev->intf);
- urb->dev = NULL;
entry->state = tx_done;
defer_bh(dev, skb, &dev->txq);
}
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 related
* Re: [PATCH v3 4/4] ath9k: Support ethtool getstats api.
From: Ben Greear @ 2012-03-21 16:20 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Sujith Manoharan, linux-wireless, netdev
In-Reply-To: <1332346257.3500.9.camel@deadeye>
On 03/21/2012 09:10 AM, Ben Hutchings wrote:
> On Wed, 2012-03-21 at 12:37 +0530, Sujith Manoharan wrote:
>> Ben Greear wrote:
>>> I'd like to gather at least most stats always, so ethtool can work regardless
>>> of debugfs. But, that can be follow on patches in my opinion. If it turns
>>> out that we need another config option for this, then that is fine too.
>>
>> It would be good to have an option to compile this out. The information is
>> available via the debugfs interface, so this is basically duplicating things.
>> On APs using OpenWRT, debugfs is enabled by default, so we can just read
>> the debugfs files.
>
> ethtool is the normal way to expose extended network stats, so the
> debugfs interface should be dropped in favour of this.
No...ath9k debugfs offers a lot of additional info that is not easily
packaged into ethtool stats, and is better formatted for reading
by humans.
So, both should be kept..though we can optionally compile
out both/either/none.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Roland Dreier @ 2012-03-21 16:20 UTC (permalink / raw)
To: parav.pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <24c5b654-d6a5-418d-8187-fba4ad47a3ce-nbYkmrCdWxmgMrCBcu8zE0EOCMrvLtNR@public.gmane.org>
On Tue, Mar 20, 2012 at 3:39 PM, <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> From: Parav Pandit <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
>
> - Header file for userspace library and kernel driver interface.
> +struct ocrdma_alloc_ucontext_resp {
> + u32 dev_id;
> + u32 wqe_size;
> + u32 max_inline_data;
> + u32 dpp_wqe_size;
> + u64 ah_tbl_page;
> + u32 ah_tbl_len;
> + u32 rsvd;
> + u8 fw_ver[32];
> + u32 rqe_size;
> + u64 rsvd1;
> +} __packed;
If I'm reading this correctly, you have the 8-byte rsvd1 member at an
offset only aligned to 4 bytes, because of the __packed directive. It
would be much better to have these structures laid out so they are
naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
__packed directive, which wrecks gcc code generation in some cases.
In this particular case, it seems you could just move rqe_size into the
slot where rsvd is, and get rid of rsvd1?
> +/* user kernel communication data structures. */
> +struct ocrdma_alloc_pd_ureq {
> + u64 rsvd1;
> +} __packed;
Similar comment -- __packed is silly for a structure with one reserved
member (and which you don't seem to use anywhere)... why not just
delete this struct?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Roland Dreier @ 2012-03-21 16:14 UTC (permalink / raw)
To: parav.pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <3f46a051-ee2e-4e18-becf-60f6c023c3c6-nbYkmrCdWxmgMrCBcu8zE0EOCMrvLtNR@public.gmane.org>
> +#define ocrdma_err(format, arg...) printk(KERN_ERR format, ##arg)
I think you'd be better off using pr_err() rather than defining your own macro.
> +struct ocrdma_cq {
> + struct ib_cq ibcq;
> + struct ocrdma_dev *dev;
> + struct ocrdma_cqe *va;
> + u32 phase;
> + u32 getp; /* pointer to pending wrs to
> + * return to stack, wrap arounds
> + * at max_hw_cqe
> + */
> + u32 max_hw_cqe;
> + bool phase_change;
> + bool armed, solicited;
> + bool arm_needed;
> +
> + spinlock_t cq_lock ____cacheline_aligned; /* provide synchronization
> + * to cq polling
> + */
> + /* syncronizes cq completion handler invoked from multiple context */
> + spinlock_t comp_handler_lock ____cacheline_aligned;
You have quite a few of these alignment directives in the middle of structures.
Have you measured that leaving all these gaps gives a reall performance boost?
> + u16 id;
> + u16 eqn;
> +
> + struct ocrdma_ucontext *ucontext;
> + dma_addr_t pa;
> + u32 len;
> + atomic_t use_cnt;
> +
> + /* head of all qp's sq and rq for which cqes need to be flushed
> + * by the software.
> + */
> + struct list_head sq_head, rq_head;
> +};
> +#define OCRDMA_GET_NUM_POSTED_SHIFT_VAL(qp) \
> + (((qp->dev->nic_info.dev_family == OCRDMA_GEN2_FAMILY) && \
> + (qp->id < 64)) ? 24 : 16)
In general it's better to use inline functions when possible
instead of macros, which are less type-safe and harder to read.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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: use-after-free in usbnet
From: Alan Stern @ 2012-03-21 16:12 UTC (permalink / raw)
To: Ming Lei
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team, Dave Jones
In-Reply-To: <CACVXFVP+U1k7JFTmbabF-k8F3bO9zc58c3tLG6=1nQPcrR9p1g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 2679 bytes --]
On Wed, 21 Mar 2012, Ming Lei wrote:
> On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLrNAH6kLmebB@public.gmane.orgdu> wrote:
> > On Wed, 21 Mar 2012, Ming Lei wrote:
> >
> >> Looks it is a general issue about usb_hcd_unlink_urb.
> >>
> >> Alan, how about increasing URB reference count before calling unlink1
> >> inside usb_hcd_unlink_urb to fix this kind of problem?
> >
> > No, that won't fix the problem. The URB could complete and be
> > deallocated even before usb_hcd_unlink_urb() is called, so nothing that
> > function can do will prevent the error.
>
> IMO, driver should not call usb_hcd_unlink_urb after urb is freed from
> the driver,
> but this problem is that URB may be freed during usb_hcd_unlink_urb.
Drivers don't call usb_hcd_unlink_urb; they call usb_unlink_urb. This
sort of thing can happen:
Driver Interrupt handler
------ -----------------
call usb_unlink_urb
URB completion interrupt occurs
call usb_hcd_giveback_urb
completion routine calls usb_free_urb
URB is deallocated
call usb_hcd_unlink_urb
try to increment URB's refcount
oops because URB is gone
> In fact, it is allowed that usb_free_urb is called inside .complete handler,
> at least as said in Documentation/URB.txt:
>
> "You may free an urb that you've submitted,..."
>
> So looks reasonable to increase the URB reference count before calling
> unlink1(), just like that done inside usb_hcd_flush_endpoint(). And I
> think it is a general solution for avoiding this kind of problem.
It will not solve the problem illustrated above. The driver must avoid
freeing the URB before usb_unlink_urb returns. In this case,
increasing the refcount around the unlink call would work.
> > It is the caller's responsibility to make sure that the URB does not
> > get freed before usb_unlink_urb() or usb_kill_urb() returns. We
> > probably should mention this in the kerneldoc...
>
> If so, looks it is a bit contrary with Documentation/URB.txt, also
> this may add extra constraint(maybe unnecessary) to the driver.
It's not contradictory. You may indeed free an URB that you have
submitted, so long as you don't free it while usb_unlink_urb (or
related routines like usb_kill_urb) is running.
The extra constraint on the driver is indeed necessary. However the
driver can avoid complications by using anchors.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 4/4] ath9k: Support ethtool getstats api.
From: Ben Hutchings @ 2012-03-21 16:10 UTC (permalink / raw)
To: Sujith Manoharan; +Cc: Ben Greear, linux-wireless, netdev
In-Reply-To: <20329.32334.162938.169995@gargle.gargle.HOWL>
On Wed, 2012-03-21 at 12:37 +0530, Sujith Manoharan wrote:
> Ben Greear wrote:
> > I'd like to gather at least most stats always, so ethtool can work regardless
> > of debugfs. But, that can be follow on patches in my opinion. If it turns
> > out that we need another config option for this, then that is fine too.
>
> It would be good to have an option to compile this out. The information is
> available via the debugfs interface, so this is basically duplicating things.
> On APs using OpenWRT, debugfs is enabled by default, so we can just read
> the debugfs files.
ethtool is the normal way to expose extended network stats, so the
debugfs interface should be dropped in favour of this.
Ben.
--
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
* Re: [PATCH 0/9] ocrdma: Driver for Emulex OneConnect RDMA
From: Roland Dreier @ 2012-03-21 16:09 UTC (permalink / raw)
To: parav.pandit; +Cc: linux-rdma, netdev
In-Reply-To: <cdc8267d-7cc4-4364-ade7-0b10f02a7c49@exht1.ad.emulex.com>
Overall looks pretty good... some comments on individual patches coming.
- R.
^ permalink raw reply
* Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Richard Cochran @ 2012-03-21 16:08 UTC (permalink / raw)
To: chetan loke
Cc: netdev, e1000-devel, jacob.e.keller, jeffrey.t.kirsher,
john.ronciak, john.stultz, tglx
In-Reply-To: <CAAsGZS6=UsyiGK=ZOwRuY0CNR7aOFLrwpn-2hXEtQJbWEULciA@mail.gmail.com>
On Wed, Mar 21, 2012 at 11:00:59AM -0400, chetan loke wrote:
>
> Once PHC->gettime goes live(aka exported to user space), we can't
> really control how users will use it in their applications. There
> could be 100+ apps all trying to get real-time from the network to do
> some time-keeping stuff. They might pound the ioctls at high rate. The
> last thing we would want is to self-induce a light weight DOS.
Well, if people want to write programs that make no sense at all,
then I can cannot stop them. There really isn't any point in general
applications using the PHC directly. You can easily synchronize the
system time to the PHC to within a few microseconds. Just the error
from reading the clock (due to various kinds of latency) is much
larger than that.
Also, although you might be able to optimize clock_gettime performance
for a PCI card, this will never work for PHY based devices, which, by
the way, offer superior PTP time stamping. So, in general, applications
should stick to reading the system time.
We still need to get some more tools out there in order to support the
PHC->system synchronization, but that is not too far off. That is what
I am working on now, and I don't think optimizing the igb is worth the
effort. If you want to try it, please go right ahead, but adding the
PPS would be much more useful IMHO.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH] sky2: override for PCI legacy power management
From: Knut Petersen @ 2012-03-21 15:56 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David S. Miller, Linus Torvalds, arekm, Jared, dilieto,
linux-kernel, netdev
In-Reply-To: <20120321083205.360a7a3b@nehalam.linuxnetplumber.net>
Thanks a lot!
> Some BIOS's don't setup power management correctly (what else is
> new) and don't allow use of PCI Express power control. Add a special
> exception module parameter to allow working around this issue.
> Based on slightly different patch by Knut Petersen.
>
> Reported-by: Arkadiusz Miskiewicz<arekm@maven.pl>
> Signed-off-by: Stephen Hemminger<shemminger@vyatta.com>
>
^ permalink raw reply
* [PATCH] sky2: override for PCI legacy power management
From: Stephen Hemminger @ 2012-03-21 15:32 UTC (permalink / raw)
To: Knut Petersen, David S. Miller
Cc: Linus Torvalds, arekm, Jared, dilieto, linux-kernel, netdev
In-Reply-To: <4F69BE2E.7070903@t-online.de>
Some BIOS's don't setup power management correctly (what else is
new) and don't allow use of PCI Express power control. Add a special
exception module parameter to allow working around this issue.
Based on slightly different patch by Knut Petersen.
Reported-by: Arkadiusz Miskiewicz <arekm@maven.pl>
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
Patch against -net (ie. 3.3.0)
--- a/drivers/net/ethernet/marvell/sky2.c 2012-01-10 10:56:56.855156017 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c 2012-03-21 08:25:52.400929532 -0700
@@ -95,6 +95,10 @@ static int disable_msi = 0;
module_param(disable_msi, int, 0);
MODULE_PARM_DESC(disable_msi, "Disable Message Signaled Interrupt (MSI)");
+static int legacy_pme = 0;
+module_param(legacy_pme, int, 0);
+MODULE_PARM_DESC(legacy_pme, "Legacy power management");
+
static DEFINE_PCI_DEVICE_TABLE(sky2_id_table) = {
{ PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9000) }, /* SK-9Sxx */
{ PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9E00) }, /* SK-9Exx */
@@ -867,6 +871,13 @@ static void sky2_wol_init(struct sky2_po
/* Disable PiG firmware */
sky2_write16(hw, B0_CTST, Y2_HW_WOL_OFF);
+ /* Needed by some broken BIOSes, use PCI rather than PCI-e for WOL */
+ if (legacy_pme) {
+ u32 reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
+ reg1 |= PCI_Y2_PME_LEGACY;
+ sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
+ }
+
/* block receiver */
sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
sky2_read32(hw, B0_CTST);
^ permalink raw reply
* Re: [PATCH V1 5/6] net/mlx4_en: sk_prio <=> UP for untagged traffic
From: Amir Vadai @ 2012-03-21 15:18 UTC (permalink / raw)
To: John Fastabend
Cc: David S. Miller, netdev, Roland Dreier, Oren Duer, Amir Vadai
In-Reply-To: <4F69E3AD.2000501@intel.com>
On 03/21/2012 04:20 PM, John Fastabend wrote:
> On 3/21/2012 2:25 AM, Amir Vadai wrote:
>> From: Amir Vadai<amirv@mellanox.co.il>
>>
>> Since vlan egress map is only good for tagged traffic, need to have other
>> mapping to be used by untagged traffic.
>> For that, the driver uses sch_mqprio mapping. This mapping could be set by
>> using tc tool from iproute2 package.
>> Mapped UP will be used by the HW for QoS purposes, but won't go out on the
>> wire.
>>
>> Signed-off-by: Amir Vadai<amirv@mellanox.com>
>> ---
>
> [...]
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index 445a771..f228728 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -570,15 +570,15 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, struct sk_buff *sk
>>
>> u16 mlx4_en_select_queue(struct net_device *dev, struct sk_buff *skb)
>> {
>> - u16 vlan_tag = 0;
>> + int up = -1;
>>
>> - /* If we support per priority flow control and the packet contains
>> - * a vlan tag, send the packet to the TX ring assigned to that priority
>> - */
>> - if (vlan_tx_tag_present(skb)) {
>> - vlan_tag = vlan_tx_tag_get(skb);
>> - return MLX4_EN_NUM_TX_RINGS + (vlan_tag>> 13);
>> - }
>> + if (vlan_tx_tag_present(skb))
>> + up = (vlan_tx_tag_get(skb)>> 13);
>> + else if (dev->num_tc)
>> + up = netdev_get_prio_tc_map(dev, skb->priority);
>> +
>> + if (up>= 0)
>> + return MLX4_EN_NUM_TX_RINGS + up;
>
> I expected the else case covered by the netdev_set_tc_queue() setup above? Did
> I miss something.
>
It is me who missed something, will fix it in V2
>>
>> return __skb_tx_hash(dev, skb, MLX4_EN_NUM_TX_RINGS);
>> }
>
- Amir
^ permalink raw reply
* Re: [PATCH v3 4/4] ath9k: Support ethtool getstats api.
From: Ben Greear @ 2012-03-21 15:12 UTC (permalink / raw)
To: Sujith Manoharan
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20329.32334.162938.169995-4mDQ13Tdud8Jw5R7aSpS0dP8p4LwMBBS@public.gmane.org>
On 03/21/2012 12:07 AM, Sujith Manoharan wrote:
> Ben Greear wrote:
>> I'd like to gather at least most stats always, so ethtool can work regardless
>> of debugfs. But, that can be follow on patches in my opinion. If it turns
>> out that we need another config option for this, then that is fine too.
>
> It would be good to have an option to compile this out. The information is
> available via the debugfs interface, so this is basically duplicating things.
> On APs using OpenWRT, debugfs is enabled by default, so we can just read
> the debugfs files.
Ok. Hopefully at least the first 3 patches will go in, and when they do
I'll post some more patches for consideration.
Thanks,
Ben
--
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc http://www.candelatech.com
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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: [REGRESSION][PATCH] Fix an old sky2 WOL regression
From: Stephen Hemminger @ 2012-03-21 15:08 UTC (permalink / raw)
To: Knut Petersen
Cc: Linus Torvalds, Andrew Morton, David S. Miller, arekm, Jared,
dilieto, linux-kernel, netdev
In-Reply-To: <4F69BE2E.7070903@t-online.de>
On Wed, 21 Mar 2012 12:40:30 +0100
Knut Petersen <Knut_Petersen@t-online.de> wrote:
> Sky2 Wake on LAN is broken since February 2010 on a number of systems.
> Yes. More than two years.
>
> We know about the problem and the cause since October 2010
> (Bugzilla bug #19492). It´s commit 87b09f1f25cd1e01d7c50bf423c7fe33027d7511.
>
> Stephen, David: You signed off that commit.
>
> Andrew: You called it a regression in October 2010.
>
> It has been proposed to revert the commit that caused the problem.
> Nothing happened.
>
> I proposed to re-establish the old code for dmi_match()ed systems.
> Without success.
>
> Now it is proposed to re-establish the old code as a configuration option.
> If nothing happens again I will propose a module parameter ;-)
>
> Stephen, I don´t want to be a pain in the neck, and it is not my intention
> to offend you by my "attitude". But I simply cannot understand why this
> know regression is not fixed. The bit we talk about is documented,
> and in fact it was set for a number of kernel versions unconditionally.
> Nobody complained about ruined hardware or minor problems.
>
> The systems affected are old enough that no manufacturer cares about
> them, but they are still quite usable for a lot of jobs (kernel 3.3 compile
> time here is below 15 minutes).
>
> If there is a problem in the kernel and if we do know an easy solution,
> that solution should be commited to the kernel, no matter what is written
> in some random documentation, no matter if we could blame some
> BIOS authors. That´s the way Linux works - at least I thought so.
>
> cu,
> Knut
Config options don't work for distro's.
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-03-21 15:07 UTC (permalink / raw)
To: Greg KH; +Cc: Dave Jones, netdev, linux-usb, Fedora Kernel Team
In-Reply-To: <20120321144441.GB14043@kroah.com>
On Wed, Mar 21, 2012 at 10:44 PM, Greg KH <greg@kroah.com> wrote:
> On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote:
>> On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> > Hi,
>> >
>> > On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones <davej@redhat.com> wrote:
>> >> We've had two reports of this use after free in Fedora now recently..
>> >
>> > Could you provide output of 'dmesg' and 'lsusb -v' from the reported machine?
>>
>> Looks I have figured out why your problem is triggered.
>>
>> If the URB being unlinked is freed before usb_put_dev
>> inside usb_hcd_unlink_urb, the use-after-free will be triggered.
>> And the below patch[1] should fix the problem.
>
> With the reference counting we have, how can the urb be freed at this
> point in time? Is the driver doing wierd things with the urb reference
> counts?
The problem is that the .complete may schedule a tasklet to
free the completed URB. And the .complete may be run inside
unlink path, so the use-after-free will be triggered if the
tasklet is excuted before usb_put_dev inside usb_hcd_unlink_urb.
>
>> Also there is another bug in tx_complete() of usbnet, the line below
>>
>> urb->dev = NULL;
>>
>> should be removed to avoid possible oops or memory leak in unlink path.
>>
>> Please test the patch if you can reproduce the problem.
>>
>> [1],
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 59681f0..4f4e028 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -592,7 +592,9 @@ static int unlink_urbs (struct usbnet *dev, struct
>> sk_buff_head *q)
>> spin_unlock_irqrestore(&q->lock, flags);
>> // during some PM-driven resume scenarios,
>> // these (async) unlinks complete immediately
>> + local_bh_disable();
>> retval = usb_unlink_urb (urb);
>> + local_bh_enable();
>
> That doesn't seem right, as you point out in your follow-up message.
> This shouldn't be needed, unless you are doing some really wierd things
> with the urb :(
Looks the driver doesn't do any wierd things, as said above.
Thanks,
--
Ming Lei
^ permalink raw reply
* [PATCH] net: add device tree support for DaVinci MDIO
From: s-paulraj-l0cyMroinI0 @ 2012-03-21 15:05 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA,
davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ
From: Sandeep Paulraj <s-paulraj-l0cyMroinI0@public.gmane.org>
This patch adds device tree support in the DaVinci MDIO driver.
Signed-off-by: Sandeep Paulraj <s-paulraj-l0cyMroinI0@public.gmane.org>
---
.../devicetree/bindings/net/davinci_mdio.txt | 12 ++++++++++++
drivers/net/ethernet/ti/davinci_mdio.c | 7 +++++++
2 files changed, 19 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/davinci_mdio.txt
diff --git a/Documentation/devicetree/bindings/net/davinci_mdio.txt b/Documentation/devicetree/bindings/net/davinci_mdio.txt
new file mode 100644
index 0000000..cbe499f
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/davinci_mdio.txt
@@ -0,0 +1,12 @@
+MDIO on DaVinci SOCs
+
+Currently defined compatibles:
+- ti,davinci_mdio
+
+Example:
+
+mdio: mdio@2090300 {
+ compatible = "ti,davinci_mdio";
+ reg = <0x2090300 0x100>;
+ };
+
diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 7615040..362644d 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -34,6 +34,7 @@
#include <linux/clk.h>
#include <linux/err.h>
#include <linux/io.h>
+#include <linux/of.h>
#include <linux/davinci_emac.h>
/*
@@ -449,11 +450,17 @@ static const struct dev_pm_ops davinci_mdio_pm_ops = {
.resume = davinci_mdio_resume,
};
+static struct of_device_id __devinitdata of_match[] = {
+ { .compatible = "ti,davinci_mdio", },
+ {},
+};
+
static struct platform_driver davinci_mdio_driver = {
.driver = {
.name = "davinci_mdio",
.owner = THIS_MODULE,
.pm = &davinci_mdio_pm_ops,
+ .of_match_table = of_match,
},
.probe = davinci_mdio_probe,
.remove = __devexit_p(davinci_mdio_remove),
--
1.7.4.1
^ permalink raw reply related
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-03-21 15:02 UTC (permalink / raw)
To: Alan Stern
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team, Dave Jones
In-Reply-To: <Pine.LNX.4.44L0.1203211031490.1369-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Wed, 21 Mar 2012, Ming Lei wrote:
>
>> Looks it is a general issue about usb_hcd_unlink_urb.
>>
>> Alan, how about increasing URB reference count before calling unlink1
>> inside usb_hcd_unlink_urb to fix this kind of problem?
>
> No, that won't fix the problem. The URB could complete and be
> deallocated even before usb_hcd_unlink_urb() is called, so nothing that
> function can do will prevent the error.
IMO, driver should not call usb_hcd_unlink_urb after urb is freed from
the driver,
but this problem is that URB may be freed during usb_hcd_unlink_urb.
In fact, it is allowed that usb_free_urb is called inside .complete handler,
at least as said in Documentation/URB.txt:
"You may free an urb that you've submitted,..."
So looks reasonable to increase the URB reference count before calling
unlink1(), just like that done inside usb_hcd_flush_endpoint(). And I
think it is a general solution for avoiding this kind of problem.
>
> It is the caller's responsibility to make sure that the URB does not
> get freed before usb_unlink_urb() or usb_kill_urb() returns. We
> probably should mention this in the kerneldoc...
If so, looks it is a bit contrary with Documentation/URB.txt, also
this may add extra constraint(maybe unnecessary) to the driver.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: chetan loke @ 2012-03-21 15:00 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, e1000-devel, jacob.e.keller, jeffrey.t.kirsher,
john.ronciak, john.stultz, tglx
In-Reply-To: <20120316065537.GB2199@netboy.at.omicron.at>
On Fri, Mar 16, 2012 at 2:55 AM, Richard Cochran
<richardcochran@gmail.com> wrote:
> On Thu, Mar 15, 2012 at 01:18:26PM -0400, chetan loke wrote:
>>
>> 1) how can I use igb->PHC to set/discipline my system time?
>> a) clock_gettime(IGB_CLK_ID...);
>> b) clock_settime(REAL_TIME,...) ? Good/bad idea?
>
> This is a very crude method, and it will cause frequent jumps in your
> system clock.
Of course it's crude :). I wanted to make sure if my understanding was correct.
>> 4) possible contention on tmreg_lock?
>> I will read previous emails/patches shortly but I was thinking - what
>> if there are many readers who would like to use PHC as reference time?
>> Then do we want them to block the driver who wants to time-stamp the
>> packet?
>> igb_gettime->spin_lock::tmreg_lock <---contention---> driver
>> ->igb_systim_to_hwtstamp->spin_lock::tmreg_lock
>
> Under normal use cases, I would not expect much contention at all.
>
Once PHC->gettime goes live(aka exported to user space), we can't
really control how users will use it in their applications. There
could be 100+ apps all trying to get real-time from the network to do
some time-keeping stuff. They might pound the ioctls at high rate. The
last thing we would want is to self-induce a light weight DOS. What
Eric Dumazet mentioned in the very first patch set seems like a good
comment. seqlock or whatever it is we use for jiffies.
We shouldn't let get_time block network traffic under cases when there
is high load and large number of threads are pounding get-time.
Reader's *must* wait.
reminder to myself: once this patch is applied, tpacket_v3 will
break(not because of spinlock but in general) under specific
conditions. Fix tpacket_v3.
> HTH,
>
> Richard
Chetan
^ permalink raw reply
* Re: [PATCH v2] ethernet driver for the WIZnet W5300 chip
From: Florian Fainelli @ 2012-03-21 14:49 UTC (permalink / raw)
To: Mike Sinkovsky; +Cc: netdev, linux-kernel
In-Reply-To: <1332149037-12025-1-git-send-email-msink@permonline.ru>
Hi,
Le 03/19/12 10:23, Mike Sinkovsky a écrit :
> Based on original driver from chip manufacturer, but with many cleanups.
> Hope now it is near to mainline kernel quality.
>
> Tested and used in production with Blackfin BF531 embedded processor.
>
> Signed-off-by: Mike Sinkovsky<msink@permonline.ru>
> ---
> v2:
> - corrected handling of NET_ADDR_RANDOM flag
> - support for WIZNET_BUS_ANY mode
> - link detection using gpio
> - registers read using ethtool
> - more cleanups
>
> drivers/net/ethernet/Kconfig | 1 +
> drivers/net/ethernet/Makefile | 1 +
> drivers/net/ethernet/wiznet/Kconfig | 61 +++
> drivers/net/ethernet/wiznet/Makefile | 1 +
> drivers/net/ethernet/wiznet/w5300.c | 699 ++++++++++++++++++++++++++++++++++
> 5 files changed, 763 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/ethernet/wiznet/Kconfig
> create mode 100644 drivers/net/ethernet/wiznet/Makefile
> create mode 100644 drivers/net/ethernet/wiznet/w5300.c
>
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index 3474a61..e87313f 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -173,6 +173,7 @@ source "drivers/net/ethernet/tile/Kconfig"
> source "drivers/net/ethernet/toshiba/Kconfig"
> source "drivers/net/ethernet/tundra/Kconfig"
> source "drivers/net/ethernet/via/Kconfig"
> +source "drivers/net/ethernet/wiznet/Kconfig"
> source "drivers/net/ethernet/xilinx/Kconfig"
> source "drivers/net/ethernet/xircom/Kconfig"
>
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index 08d5f03..d24db66 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -72,5 +72,6 @@ obj-$(CONFIG_TILE_NET) += tile/
> obj-$(CONFIG_NET_VENDOR_TOSHIBA) += toshiba/
> obj-$(CONFIG_NET_VENDOR_TUNDRA) += tundra/
> obj-$(CONFIG_NET_VENDOR_VIA) += via/
> +obj-$(CONFIG_NET_VENDOR_WIZNET) += wiznet/
> obj-$(CONFIG_NET_VENDOR_XILINX) += xilinx/
> obj-$(CONFIG_NET_VENDOR_XIRCOM) += xircom/
> diff --git a/drivers/net/ethernet/wiznet/Kconfig b/drivers/net/ethernet/wiznet/Kconfig
> new file mode 100644
> index 0000000..748fa3b
> --- /dev/null
> +++ b/drivers/net/ethernet/wiznet/Kconfig
> @@ -0,0 +1,61 @@
> +#
> +# WIZnet device configuration
> +#
> +
> +config NET_VENDOR_WIZNET
> + bool "WIZnet devices"
> + default y
> + ---help---
> + If you have a network (Ethernet) card belonging to this class, say Y
> + and read the Ethernet-HOWTO, available from
> + <http://www.tldp.org/docs.html#howto>.
> +
> + Note that the answer to this question doesn't directly affect the
> + kernel: saying N will just cause the configurator to skip all
> + the questions about WIZnet devices. If you say Y, you will be asked
> + for your specific card in the following questions.
> +
> +if NET_VENDOR_WIZNET
> +
> +config WIZNET_W5300
> + tristate "WIZnet W5300 Ethernet support"
> + depends on ARM || BLACKFIN
> + ---help---
> + Support for WIZnet W5300 chips.
> +
> + W5300 is a single chip with integrated 10/100 Ethernet MAC,
> + PHY and hardware TCP/IP stack, but this driver is limited to
> + the MAC and PHY functions only, onchip TCP/IP is unused.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called w5300.
> +
> +choice
> + prompt "WIZnet interface mode"
> + depends on NET_VENDOR_WIZNET
> + default WIZNET_BUS_ANY
> +
> +config WIZNET_BUS_DIRECT
> + bool "Direct address bus mode"
> + ---help---
> + In direct address mode host system can directly access W5300 registers
> + after mapping to Memory-mapped I/O Space.
> + 0x400 bytes are required for memory space.
> +
> +config WIZNET_BUS_INDIRECT
> + bool "Indirect address bus mode"
> + ---help---
> + In indirect address mode host system indirectly accesses registers by
> + using Indirect Mode Address Register (IDM_AR) and Indirect Mode Data
> + Register (IDM_DR), which are directly mapped to Memory-mapped I/O Space.
> + Only 0x06 bytes are required for memory space.
> +
> +config WIZNET_BUS_ANY
> + bool "Select interface mode in runtime"
> + ---help---
> + If interface mode is unknown in compile time, you can selectied it
> + in runtime.
> + Performance may decrease compared to explicitly selected bus mode.
> +endchoice
> +
> +endif # NET_VENDOR_WIZNET
> diff --git a/drivers/net/ethernet/wiznet/Makefile b/drivers/net/ethernet/wiznet/Makefile
> new file mode 100644
> index 0000000..88e0a3e
> --- /dev/null
> +++ b/drivers/net/ethernet/wiznet/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_WIZNET_W5300) += w5300.o
> diff --git a/drivers/net/ethernet/wiznet/w5300.c b/drivers/net/ethernet/wiznet/w5300.c
> new file mode 100644
> index 0000000..8f7adfa
> --- /dev/null
> +++ b/drivers/net/ethernet/wiznet/w5300.c
> @@ -0,0 +1,699 @@
> +/*
> + * Ethernet driver for the WIZnet W5300 chip.
> + *
> + * Copyright (C) 2008-2009 WIZnet Co.,Ltd.
> + * Copyright (C) 2011 Taehun Kim<kth3321<at> gmail.com>
> + * Copyright (C) 2012 Mike Sinkovsky<msink@permonline.ru>
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include<linux/module.h>
> +#include<linux/kernel.h>
> +#include<linux/platform_device.h>
> +#include<linux/netdevice.h>
> +#include<linux/etherdevice.h>
> +#include<linux/ethtool.h>
> +#include<linux/skbuff.h>
> +
> +#include<linux/slab.h>
> +#include<linux/types.h>
> +#include<linux/errno.h>
> +#include<linux/delay.h>
> +#include<linux/spinlock.h>
> +
> +#include<linux/io.h>
> +#include<linux/ioport.h>
> +#include<linux/interrupt.h>
> +#include<linux/gpio.h>
> +
> +#define DRV_NAME "WIZnet W5300"
> +#define DRV_VERSION "2012-03-19"
> +
> +MODULE_DESCRIPTION(DRV_NAME "Ethernet driver v" DRV_VERSION);
> +MODULE_AUTHOR("Mike Sinkovsky<msink@permonline.ru>");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_LICENSE("GPL");
> +
> +/*
> + * Frame size is hardwired to 1514 bytes,
> + * and MTU for 802.1Q frames must me set to 1496
> + */
> +#define W5300_FRAME_SIZE 1514
> +
> +/*
> + * Device driver private data structure
> + */
> +struct w5300_private {
> + void __iomem *base;
> + int irq;
> + int link;
> +
> + spinlock_t reg_lock;
> + bool promisc_mode;
> + u16 (*read_u16) (struct w5300_private *priv, u16 addr);
> + void (*write_u16)(struct w5300_private *priv, u16 addr, u16 data);
> +
> + struct napi_struct napi;
> + struct net_device *ndev;
> +};
> +
> +/************************************************************************
> + *
> + * Lowlevel I/O functions
> + *
> + ***********************************************************************/
> +
> +/*
> + * In direct address mode host system can directly access W5300 registers
> + * after mapping to Memory-mapped I/O Space.
> + *
> + * 0x400 bytes are required for memory space.
> + */
> +static inline u16
> +read_u16_direct(struct w5300_private *priv, u16 addr)
> +{
> + return ioread16(priv->base + addr);
> +}
> +
> +static inline void
> +write_u16_direct(struct w5300_private *priv, u16 addr, u16 data)
> +{
> + iowrite16(data, priv->base + addr);
> + mmiowb();
> +}
> +
> +/*
> + * In indirect address mode host system indirectly accesses registers by
> + * using Indirect Mode Address Register (IDM_AR) and Indirect Mode Data
> + * Register (IDM_DR), which are directly mapped to Memory-mapped I/O Space.
> + * Mode Register (MR) is directly accessible.
> + *
> + * Only 0x06 bytes are required for memory space.
> + */
> +#define W5300_MR 0x00 /* Mode Register offset */
> +#define W5300_IDM_AR 0x02 /* Indirect Mode Address Register offset */
> +#define W5300_IDM_DR 0x04 /* Indirect Mode Data Register offset */
> +
> +static inline u16
> +read_u16_indirect(struct w5300_private *priv, u16 addr)
> +{
> + unsigned long flags;
> + u16 data;
> +
> + spin_lock_irqsave(&priv->reg_lock, flags);
> + write_u16_direct(priv, W5300_IDM_AR, addr);
> + data = read_u16_direct(priv, W5300_IDM_DR);
> + spin_unlock_irqrestore(&priv->reg_lock, flags);
> +
> + return data;
> +}
> +
> +static inline void
> +write_u16_indirect(struct w5300_private *priv, u16 addr, u16 data)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->reg_lock, flags);
> + write_u16_direct(priv, W5300_IDM_AR, addr);
> + write_u16_direct(priv, W5300_IDM_DR, data);
> + spin_unlock_irqrestore(&priv->reg_lock, flags);
> +}
> +
> +#if defined(CONFIG_WIZNET_BUS_DIRECT)
> +#define detect_bus_mode(priv, mem_size) do {} while(0)
> +#define read_reg_u16 read_u16_direct
> +#define write_reg_u16 write_u16_direct
> +
> +#elif defined(CONFIG_WIZNET_BUS_INDIRECT)
> +#define detect_bus_mode(priv, mem_size) do {} while(0)
> +#define read_reg_u16 read_u16_indirect
> +#define write_reg_u16 write_u16_indirect
Looks like you don't have to make such decisions at compile-time. Since
it is a platform driver, better supply this through platform_data instead.
> +
> +#else /* CONFIG_WIZNET_BUS_ANY */
> +static inline void
> +detect_bus_mode(struct w5300_private *priv, u16 mem_size)
> +{
> + if (mem_size< 0x400) {
> + netdev_info(priv->ndev, "bus mode: indirect\n");
> + priv->read_u16 = read_u16_indirect;
> + priv->write_u16 = write_u16_indirect;
> + } else {
> + netdev_info(priv->ndev, "bus mode: direct\n");
> + priv->read_u16 = read_u16_direct;
> + priv->write_u16 = write_u16_direct;
> + }
> +}
> +
> +static inline u16
> +read_reg_u16(struct w5300_private *priv, u16 addr)
> +{
> + return priv->read_u16(priv, addr);
> +}
> +
> +static inline void
> +write_reg_u16(struct w5300_private *priv, u16 addr, u16 data)
> +{
> + priv->write_u16(priv, addr, data);
> +}
> +#endif
> +
> +static inline u32
> +read_reg_u32(struct w5300_private *priv, u16 addr)
> +{
> + u32 data;
> + data = read_reg_u16(priv, addr)<< 16;
> + data |= read_reg_u16(priv, addr + 2);
> + return data;
> +}
> +
> +static inline void
> +write_reg_u32(struct w5300_private *priv, u16 addr, u32 data)
> +{
> + write_reg_u16(priv, addr, data>> 16);
> + write_reg_u16(priv, addr + 2, data);
> +}
> +
> +static inline void write_MR(struct w5300_private *priv, u16 data)
> +{
> + write_u16_direct(priv, W5300_MR, data);
> +}
> +
> +#define DEFINE_REG_RD(ADDR, TYPE, NAME) \
> +static inline TYPE read_##NAME(struct w5300_private *priv) \
> +{ \
> + return read_reg_##TYPE(priv, ADDR); \
> +}
> +#define DEFINE_REG_WR(ADDR, TYPE, NAME) \
> +static inline void write_##NAME(struct w5300_private *priv, TYPE data) \
> +{ \
> + write_reg_##TYPE(priv, ADDR, data); \
> +}
> +#define DEFINE_REG_RW(ADDR, TYPE, NAME) \
> + DEFINE_REG_RD(ADDR, TYPE, NAME) \
> + DEFINE_REG_WR(ADDR, TYPE, NAME)
> +
> +DEFINE_REG_RW(0x002, u16, IR) /* Interrupt Register */
> +DEFINE_REG_WR(0x004, u16, IMR) /* Interrupt Mask Register */
> +DEFINE_REG_WR(0x008, u32, SHARL) /* Source MAC address (0123) */
> +DEFINE_REG_WR(0x00c, u16, SHARH) /* Source MAC address (45) */
> +DEFINE_REG_WR(0x020, u32, TMSRL) /* Transmit Memory Size (0123) */
> +DEFINE_REG_WR(0x024, u32, TMSRH) /* Transmit Memory Size (4567) */
> +DEFINE_REG_WR(0x028, u32, RMSRL) /* Receive Memory Size (0123) */
> +DEFINE_REG_WR(0x02c, u32, RMSRH) /* Receive Memory Size (4567) */
> +DEFINE_REG_WR(0x030, u16, MTYPE) /* Memory Type */
> +DEFINE_REG_RD(0x0fe, u16, IDR) /* Chip ID register (=0x5300) */
> +DEFINE_REG_WR(0x200, u16, S0_MR) /* S0 Mode Register */
> +DEFINE_REG_RW(0x202, u16, S0_CR) /* S0 Command Register */
> +DEFINE_REG_WR(0x204, u16, S0_IMR) /* S0 Interrupt Mask Register */
> +DEFINE_REG_RW(0x206, u16, S0_IR) /* S0 Interrupt Register */
> +DEFINE_REG_RD(0x208, u16, S0_SSR) /* S0 Socket Status Register */
> +DEFINE_REG_WR(0x220, u32, S0_TX_WRSR) /* S0 TX Write Size Register */
> +DEFINE_REG_RD(0x224, u32, S0_TX_FSR) /* S0 TX Free Size Register */
> +DEFINE_REG_RD(0x228, u32, S0_RX_RSR) /* S0 Received data Size */
> +DEFINE_REG_WR(0x22e, u16, S0_TX_FIFO) /* S0 Transmit FIFO */
> +DEFINE_REG_RD(0x230, u16, S0_RX_FIFO) /* S0 Receive FIFO */
> +
> +/* Mode Register values */
> +#define MR_DBW (1<< 15) /* Data bus width */
> +#define MR_MPF (1<< 14) /* Mac layer pause frame */
> +#define MR_WDF(n) (n<< 11) /* Write data fetch time */
> +#define MR_RDH (1<< 10) /* Read data hold time */
> +#define MR_FS (1<< 8) /* FIFO swap */
> +#define MR_RST (1<< 7) /* S/W reset */
> +#define MR_PB (1<< 4) /* Ping block */
> +#define MR_DBS (1<< 2) /* Data bus swap */
> +#define MR_IND (1<< 0) /* Indirect mode */
> +
> +#ifdef CONFIG_WIZNET_BUS_INDIRECT
> +#define MR_VALUE (MR_WDF(7) | MR_PB | MR_IND)
> +#else
> +#define MR_VALUE (MR_WDF(7) | MR_PB)
> +#endif
> +
> +/* IR/IMR register values */
> +#define IR_S0 0x01 /* S0 interrupt */
> +
> +/* S0_MR register values */
> +#define S0_MR_CLOSE 0x00 /* Close mode */
> +#define S0_MR_MACRAW 0x04 /* MAC RAW mode (promiscous) */
> +#define S0_MR_MACRAW_MF 0x44 /* MAC RAW mode (filtered) */
> +
> +/* S0_CR register values */
> +#define S0_CR_OPEN 0x01 /* OPEN command */
> +#define S0_CR_CLOSE 0x10 /* CLOSE command */
> +#define S0_CR_SEND 0x20 /* SEND command */
> +#define S0_CR_RECV 0x40 /* RECV command */
> +
> +/* S0_IR/S0_IMR register values */
> +#define S0_IR_RECV 0x04 /* Receive interrupt */
> +
> +static void read_fifo(struct w5300_private *priv, u8 *data, int len)
> +{
> + for (; len> 0; len -= 2) {
> + u16 fifo = read_S0_RX_FIFO(priv);
> + *data++ = fifo>> 8;
> + *data++ = fifo;
> + }
> +}
> +
> +static void write_fifo(struct w5300_private *priv, u8 *data, int len)
> +{
> + for (; len> 0; len -= 2) {
> + u16 fifo = *data++<< 8;
> + fifo |= *data++;
> + write_S0_TX_FIFO(priv, fifo);
> + }
> +}
> +
> +static inline int send_command(struct w5300_private *priv, u16 cmd)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(100);
> +
> + write_S0_CR(priv, cmd);
> +
> + while (read_S0_CR(priv) != 0) {
> + if (time_after(jiffies, timeout))
> + return -EIO;
> + cpu_relax();
> + }
> +
> + return 0;
> +}
> +
> +static void write_macaddr(struct w5300_private *priv)
> +{
> + struct net_device *ndev = priv->ndev;
> + write_SHARL(priv, ndev->dev_addr[0]<< 24 |
> + ndev->dev_addr[1]<< 16 |
> + ndev->dev_addr[2]<< 8 |
> + ndev->dev_addr[3]);
> + write_SHARH(priv, ndev->dev_addr[4]<< 8 |
> + ndev->dev_addr[5]);
> +}
> +
> +static void reset_chip(struct w5300_private *priv)
> +{
> + write_MR(priv, MR_RST);
> + mdelay(5);
> + write_MR(priv, MR_VALUE);
> +
> + write_IMR(priv, 0);
> +
> + /*
> + * Configure 128K of internal memory
> + * as 64K RX fifo and 64K TX fifo
> + */
> + write_RMSRL(priv, 64<< 24);
> + write_RMSRH(priv, 0);
> + write_TMSRL(priv, 64<< 24);
> + write_TMSRH(priv, 0);
> + write_MTYPE(priv, 0x00ff);
> +
> + write_macaddr(priv);
> +}
> +
> +/***********************************************************************
> + *
> + * Device driver functions / callbacks
> + *
> + ***********************************************************************/
> +
> +static void w5300_get_drvinfo(struct net_device *ndev,
> + struct ethtool_drvinfo *info)
> +{
> + strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
> + strlcpy(info->version, DRV_VERSION, sizeof(info->version));
> + strlcpy(info->fw_version, "N/A", sizeof(info->fw_version));
> + strlcpy(info->bus_info, dev_name(ndev->dev.parent),
> + sizeof(info->bus_info));
> +}
> +
> +#define W5300_REGS_LEN 0x400
> +
> +static int w5300_get_regs_len(struct net_device *ndev)
> +{
> + return W5300_REGS_LEN;
> +}
> +
> +static void w5300_get_regs(struct net_device *ndev,
> + struct ethtool_regs *regs, void *_buf)
> +{
> + struct w5300_private *priv = netdev_priv(ndev);
> + u8 *buf = _buf;
> + u16 addr;
> + u16 data;
> +
> + regs->version = 1;
> + for (addr = 0; addr< W5300_REGS_LEN; addr += 2) {
> + switch (addr& 0x23f) {
> + case 0x22e: /* don't read TX_FIFO register! */
> + case 0x230: /* don't read RX_FIFO register! */
> + data = 0xffff;
> + break;
> + default:
> + data = read_reg_u16(priv, addr);
> + break;
> + }
> + *buf++ = data>> 8;
> + *buf++ = data;
> + }
> +}
> +
> +static void w5300_tx_timeout(struct net_device *ndev)
> +{
> + struct w5300_private *priv = netdev_priv(ndev);
> +
> + netdev_err(ndev, "Transmit timeout!\n");
> +
> + ndev->stats.tx_errors++;
> + reset_chip(priv);
> + netif_wake_queue(ndev);
> +}
> +
> +static int w5300_start_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct w5300_private *priv = netdev_priv(ndev);
> +
> + if (unlikely(read_S0_TX_FSR(priv)< skb->len)) {
> + ndev->stats.tx_dropped++;
> + return NETDEV_TX_BUSY;
> + }
> +
> + write_fifo(priv, skb->data, skb->len);
> + write_S0_TX_WRSR(priv, skb->len);
> + send_command(priv, S0_CR_SEND);
> +
> + ndev->stats.tx_packets++;
> + ndev->stats.tx_bytes += skb->len;
> + dev_kfree_skb(skb);
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static int w5300_napi_poll(struct napi_struct *napi, int budget)
> +{
> + struct w5300_private *priv =
> + container_of(napi, struct w5300_private, napi);
> + struct net_device *ndev = priv->ndev;
> + struct sk_buff *skb;
> + u16 rx_frame_size;
> + int rx_count;
> +
> + for (rx_count = 0; rx_count< budget; rx_count++) {
> + u32 rx_fifo_size = read_S0_RX_RSR(priv);
> + if (rx_fifo_size == 0)
> + break;
> +
> + rx_frame_size = read_S0_RX_FIFO(priv);
> +
> + skb = netdev_alloc_skb(ndev, NET_IP_ALIGN +
> + roundup(rx_frame_size, 2));
> + if (unlikely(!skb)) {
> + int len = rx_frame_size + 4;
> + for (; len> 0; len -= 2)
> + read_S0_RX_FIFO(priv);
> + ndev->stats.rx_dropped++;
> + return -ENOMEM;
> + }
> +
> + skb_reserve(skb, NET_IP_ALIGN);
> + skb_put(skb, rx_frame_size);
> + read_fifo(priv, skb->data, rx_frame_size);
> + read_S0_RX_FIFO(priv);
> + read_S0_RX_FIFO(priv);
> +
> + skb->protocol = eth_type_trans(skb, ndev);
> + netif_receive_skb(skb);
> + ndev->stats.rx_packets++;
> + ndev->stats.rx_bytes += rx_frame_size;
> + }
> +
> + if (rx_count< budget) {
> + write_IMR(priv, IR_S0);
> + napi_complete(napi);
> + }
> +
> + return rx_count;
> +}
> +
> +static irqreturn_t w5300_start_rx(int irq, void *ndev_instance)
> +{
> + struct net_device *ndev = ndev_instance;
> + struct w5300_private *priv = netdev_priv(ndev);
> +
> + write_S0_IR(priv, S0_IR_RECV);
> +
> + if (napi_schedule_prep(&priv->napi)) {
> + write_IMR(priv, 0);
> + __napi_schedule(&priv->napi);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t w5300_detect_link(int irq, void *ndev_instance)
> +{
> + struct net_device *ndev = ndev_instance;
> + struct w5300_private *priv = netdev_priv(ndev);
> +
> + if (netif_running(ndev)) {
> + if (gpio_get_value(priv->link) == 0)
> + netif_carrier_off(ndev);
> + else
> + netif_carrier_on(ndev);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void w5300_set_rx_mode(struct net_device *ndev)
> +{
> + struct w5300_private *priv = netdev_priv(ndev);
> + bool set_promisc = (ndev->flags& IFF_PROMISC) != 0;
> + int mode = set_promisc ? S0_MR_MACRAW : S0_MR_MACRAW_MF;
> +
> + if (priv->promisc_mode != set_promisc) {
> + priv->promisc_mode = set_promisc;
> + write_S0_MR(priv, mode);
> + send_command(priv, S0_CR_OPEN);
> + }
> +}
> +
> +static int w5300_set_macaddr(struct net_device *ndev, void *addr)
> +{
> + struct w5300_private *priv = netdev_priv(ndev);
> + struct sockaddr *sock_addr = addr;
> +
> + if (!is_valid_ether_addr(sock_addr->sa_data))
> + return -EADDRNOTAVAIL;
> + memcpy(ndev->dev_addr, sock_addr->sa_data, ETH_ALEN);
> + ndev->addr_assign_type&= ~NET_ADDR_RANDOM;
> + write_macaddr(priv);
> + return 0;
> +}
> +
> +static int w5300_open(struct net_device *ndev)
> +{
> + struct w5300_private *priv = netdev_priv(ndev);
> + int mode = priv->promisc_mode ? S0_MR_MACRAW : S0_MR_MACRAW_MF;
> +
> + if (!is_valid_ether_addr(ndev->dev_addr))
> + return -EINVAL;
> +
> + write_S0_IMR(priv, S0_IR_RECV);
> + write_S0_MR(priv, mode);
> + send_command(priv, S0_CR_OPEN);
> + write_IMR(priv, IR_S0);
> +
> + napi_enable(&priv->napi);
> + netif_start_queue(ndev);
> + if (priv->link< 0 || gpio_get_value(priv->link))
> + netif_carrier_on(ndev);
> + return 0;
> +}
> +
> +static int w5300_stop(struct net_device *ndev)
> +{
> + struct w5300_private *priv = netdev_priv(ndev);
> +
> + write_IMR(priv, 0);
> + write_S0_CR(priv, S0_CR_CLOSE);
> +
> + netif_carrier_off(ndev);
> + netif_stop_queue(ndev);
> + napi_disable(&priv->napi);
> + return 0;
> +}
> +
> +static const struct ethtool_ops w5300_ethtool_ops = {
> + .get_drvinfo = w5300_get_drvinfo,
> + .get_regs_len = w5300_get_regs_len,
> + .get_regs = w5300_get_regs,
> +};
> +
> +static const struct net_device_ops w5300_netdev_ops = {
> + .ndo_open = w5300_open,
> + .ndo_stop = w5300_stop,
> + .ndo_start_xmit = w5300_start_tx,
> + .ndo_tx_timeout = w5300_tx_timeout,
> + .ndo_set_rx_mode = w5300_set_rx_mode,
> + .ndo_set_mac_address = w5300_set_macaddr,
> + .ndo_validate_addr = eth_validate_addr,
> + .ndo_change_mtu = eth_change_mtu,
> +};
> +
> +static int __devinit w5300_hw_probe(struct platform_device *pdev)
> +{
> + struct device *dev =&pdev->dev;
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct w5300_private *priv = netdev_priv(ndev);
> + const char *name = netdev_name(ndev);
> + struct resource *link;
> + struct resource *mem;
> + int mem_size;
> + int irq;
> + int ret;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!mem)
> + return -ENXIO;
> + mem_size = resource_size(mem);
> + if (!devm_request_mem_region(dev, mem->start, mem_size, name))
> + return -EBUSY;
> + priv->base = devm_ioremap(dev, mem->start, mem_size);
> + if (!priv->base)
> + return -EBUSY;
> +
> + spin_lock_init(&priv->reg_lock);
> + detect_bus_mode(priv, mem_size);
> + reset_chip(priv);
> + if (read_IDR(priv) != 0x5300)
> + return -ENODEV;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq< 0)
> + return irq;
> + ret = devm_request_irq(dev, irq, w5300_start_rx,
> + IRQ_TYPE_LEVEL_LOW, name, ndev);
> + if (ret< 0)
> + return ret;
> + priv->irq = irq;
> +
> + link = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + if (!link) {
> + priv->link = -1;
> + } else {
> + char *link_name = devm_kzalloc(dev, 16, GFP_KERNEL);
> + snprintf(link_name, 16, "%s-link", name);
> + priv->link = link->start;
> + if (request_irq(gpio_to_irq(priv->link), w5300_detect_link,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> + link_name, priv->ndev)< 0)
> + priv->link = -1;
> + }
Please implement phylib to properly report the link state to the
networking stack and ethtool.
> +
> + netdev_info(ndev, "at 0x%llx irq %d\n", (u64)mem->start, irq);
> + return 0;
> +}
> +
> +static int __devinit w5300_probe(struct platform_device *pdev)
> +{
> + struct net_device *ndev;
> + struct w5300_private *priv;
> + int ret;
> +
> + ndev = alloc_etherdev(sizeof(*priv));
> + if (!ndev)
> + return -ENOMEM;
> + SET_NETDEV_DEV(ndev,&pdev->dev);
> + platform_set_drvdata(pdev, ndev);
> + priv = netdev_priv(ndev);
> + priv->ndev = ndev;
> +
> + ether_setup(ndev);
> + ndev->netdev_ops =&w5300_netdev_ops;
> + ndev->ethtool_ops =&w5300_ethtool_ops;
> + ndev->watchdog_timeo = 2 * HZ;
> + netif_napi_add(ndev,&priv->napi, w5300_napi_poll, 16);
> + ret = register_netdev(ndev);
> + if (ret< 0)
> + goto fail;
> +
> + random_ether_addr(ndev->dev_addr);
> + ndev->addr_assign_type |= NET_ADDR_RANDOM;
Allow platform_data to pass a valid MAC address to this driver instead
of defaulting to random unconditionnaly.
> + ret = w5300_hw_probe(pdev);
> + if (ret< 0)
> + goto fail;
> +
> + return 0;
> +
> +fail: netdev_info(ndev, "probe failed (%d)\n", ret);
> + unregister_netdev(ndev);
> + free_netdev(ndev);
> + platform_set_drvdata(pdev, NULL);
> + return ret;
> +}
> +
> +static int __devexit w5300_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> +
> + unregister_netdev(ndev);
> + free_netdev(ndev);
> + platform_set_drvdata(pdev, NULL);
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int w5300_suspend(struct platform_device *pdev, pm_message_t mesg)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct w5300_private *priv = netdev_priv(ndev);
> +
> + if (netif_running(ndev)) {
> + netif_carrier_off(ndev);
> + netif_device_detach(ndev);
> +
> + write_IMR(priv, 0);
> + send_command(priv, S0_CR_CLOSE);
> + }
> + return 0;
> +}
> +
> +static int w5300_resume(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct w5300_private *priv = netdev_priv(ndev);
> + int mode = priv->promisc_mode ? S0_MR_MACRAW : S0_MR_MACRAW_MF;
> +
> + if (netif_running(ndev)) {
> + reset_chip(priv);
> + write_S0_MR(priv, mode);
> + send_command(priv, S0_CR_OPEN);
> + write_IMR(priv, IR_S0);
> +
> + netif_device_attach(ndev);
> + if (priv->link< 0 || gpio_get_value(priv->link))
> + netif_carrier_on(ndev);
> + }
> + return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static struct platform_driver wiznet_w5300_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + },
> + .probe = w5300_probe,
> + .remove = __devexit_p(w5300_remove),
> +#ifdef CONFIG_PM
> + .suspend = w5300_suspend,
> + .resume = w5300_resume,
> +#endif
> +};
> +
> +module_platform_driver(wiznet_w5300_driver);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: use-after-free in usbnet
From: Greg KH @ 2012-03-21 14:44 UTC (permalink / raw)
To: Ming Lei; +Cc: Dave Jones, netdev, linux-usb, Fedora Kernel Team
In-Reply-To: <CACVXFVP9-Ao_UyBoxkxzq5dg8TfkZ9CECqEUFW5bD_JRzfQybw@mail.gmail.com>
On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote:
> On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> > Hi,
> >
> > On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones <davej@redhat.com> wrote:
> >> We've had two reports of this use after free in Fedora now recently..
> >
> > Could you provide output of 'dmesg' and 'lsusb -v' from the reported machine?
>
> Looks I have figured out why your problem is triggered.
>
> If the URB being unlinked is freed before usb_put_dev
> inside usb_hcd_unlink_urb, the use-after-free will be triggered.
> And the below patch[1] should fix the problem.
With the reference counting we have, how can the urb be freed at this
point in time? Is the driver doing wierd things with the urb reference
counts?
> Also there is another bug in tx_complete() of usbnet, the line below
>
> urb->dev = NULL;
>
> should be removed to avoid possible oops or memory leak in unlink path.
>
> Please test the patch if you can reproduce the problem.
>
> [1],
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 59681f0..4f4e028 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -592,7 +592,9 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
> spin_unlock_irqrestore(&q->lock, flags);
> // during some PM-driven resume scenarios,
> // these (async) unlinks complete immediately
> + local_bh_disable();
> retval = usb_unlink_urb (urb);
> + local_bh_enable();
That doesn't seem right, as you point out in your follow-up message.
This shouldn't be needed, unless you are doing some really wierd things
with the urb :(
greg k-h
^ permalink raw reply
* Re: use-after-free in usbnet
From: Alan Stern @ 2012-03-21 14:35 UTC (permalink / raw)
To: Ming Lei; +Cc: netdev, linux-usb, Fedora Kernel Team, Dave Jones
In-Reply-To: <CACVXFVPK9vj8oqjr1SFr2MgoQn-2XoDqyb95oW+7LCHT7020Ow@mail.gmail.com>
On Wed, 21 Mar 2012, Ming Lei wrote:
> Looks it is a general issue about usb_hcd_unlink_urb.
>
> Alan, how about increasing URB reference count before calling unlink1
> inside usb_hcd_unlink_urb to fix this kind of problem?
No, that won't fix the problem. The URB could complete and be
deallocated even before usb_hcd_unlink_urb() is called, so nothing that
function can do will prevent the error.
It is the caller's responsibility to make sure that the URB does not
get freed before usb_unlink_urb() or usb_kill_urb() returns. We
probably should mention this in the kerneldoc...
Alan Stern
^ permalink raw reply
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