* 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 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Roland Dreier @ 2012-03-21 16:45 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>
On Tue, Mar 20, 2012 at 3:39 PM, <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> +struct ocrdma_queue_info {
> + void *va;
> + dma_addr_t dma;
> + u32 size;
> + u16 len;
> + u16 entry_size; /* Size of an element in the queue */
> + u16 id; /* qid, where to ring the doorbell. */
> + u16 head, tail;
> + bool created;
> + atomic_t used; /* Number of valid elements in the queue */
> +};
Forgot to mention this before... the only place the used member is touched
that I can find is
> +static inline void ocrdma_mq_inc_head(struct ocrdma_dev *dev)
> +{
> + dev->mq.sq.head = (dev->mq.sq.head + 1) & (OCRDMA_MQ_LEN - 1);
> + atomic_inc(&dev->mq.sq.used);
> +}
but I don't see anywhere that it gets read. Can "used" be deleted?
- R.
--
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
* [PATCH] net: fix napi_reuse_skb() skb reserve
From: Eric Dumazet @ 2012-03-21 16:58 UTC (permalink / raw)
To: David Miller, Herbert Xu; +Cc: netdev
napi->skb is allocated in napi_get_frags() using
netdev_alloc_skb_ip_align(), with a reserve of NET_SKB_PAD +
NET_IP_ALIGN bytes.
However, when such skb is recycled in napi_reuse_skb(), it ends with a
reserve of NET_IP_ALIGN which is suboptimal.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
net/core/dev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 0f3eb7d..452db70 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3560,7 +3560,8 @@ EXPORT_SYMBOL(napi_gro_receive);
static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
{
__skb_pull(skb, skb_headlen(skb));
- skb_reserve(skb, NET_IP_ALIGN - skb_headroom(skb));
+ /* restore the reserve we had after netdev_alloc_skb_ip_align() */
+ skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN - skb_headroom(skb));
skb->vlan_tci = 0;
skb->dev = napi->dev;
skb->skb_iif = 0;
^ permalink raw reply related
* Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Richard Cochran @ 2012-03-21 17:00 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. What
> Eric Dumazet mentioned in the very first patch set seems like a good
> comment. seqlock or whatever it is we use for jiffies.
Can you please explain how using a seqlock could help here?
Thanks,
Richard
^ 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 17:02 UTC (permalink / raw)
To: Richard Cochran
Cc: e1000-devel, netdev, john.ronciak, john.stultz, jacob.e.keller,
tglx
In-Reply-To: <20120321160817.GB1941@netboy.at.omicron.at>
On Wed, Mar 21, 2012 at 12:08 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
> 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
I thought the core patches enables using PHC as a reference time, no?
So that seems to be contradicting the PHC API. If there's no point
then why are we exporting PHC->get_time as a generic interface? May be
just limit get_time interface to something like ethtool?
> 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.
>
Once, the clocks are in-sync, it's not an error - but *somewhat
fixed* latency. There are users who don't want to spend extra money
for expensive GPS time-sync stuff but yet have enough CPUs such that
they can dedicate 1 CPU for book-keeping. For such users, this
*somewhat fixed* latency should be constant over a period of time even
when other CPUs are processing traffic at line rate.
> 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,
Sounds good on the tools part. But if applications should stick to
reading sys-time then either we shouldn't export the API or export it
selectively.If there's an API then user-space guys will use it. For
this discussion, lets focus on PCI cards because that is what this
patch talks about. Also the majority of the deployment will be LOM or
PCI cards.
>
> Thanks,
> Richard
>
thanks
Chetan
------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* RE: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Keller, Jacob E @ 2012-03-21 17:06 UTC (permalink / raw)
To: Richard Cochran, chetan loke
Cc: netdev@vger.kernel.org, e1000-devel@lists.sourceforge.net,
Kirsher, Jeffrey T, Ronciak, John, john.stultz@linaro.org,
tglx@linutronix.de
In-Reply-To: <20120321160817.GB1941@netboy.at.omicron.at>
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, March 21, 2012 9:08 AM
> To: chetan loke
> Cc: netdev@vger.kernel.org; e1000-devel@lists.sourceforge.net; Keller, Jacob
> E; Kirsher, Jeffrey T; Ronciak, John; john.stultz@linaro.org;
> tglx@linutronix.de
> Subject: Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the
> timecompare method
>
> 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
>
I agree with Chetan. I think it would be best to make sure the correct form of locking is done, as we are providing an interface to the user. Using a seqlock would allow for preventing the ioctls from blocking the hardware timestamp code.
It's a fairly simple change for the gettime function (the most likely culprit to be hammered) by changing it to use timecounter_cyc2time function instead of timecounter_read. (as long as timecounter_read is called at least every 1/2 the system time overflow, which it should be due to the work task.)
With that change, then the section use a seqlock (along with the section for checking hardware timestamps). Other places would do the full write lock.
I do agree with Richard about supporting the PPS if possible, as that is much more useful as a general clock synchronization tool.
- Jake
^ permalink raw reply
* RE: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Keller, Jacob E @ 2012-03-21 17:09 UTC (permalink / raw)
To: Richard Cochran, chetan loke
Cc: netdev@vger.kernel.org, e1000-devel@lists.sourceforge.net,
Kirsher, Jeffrey T, Ronciak, John, john.stultz@linaro.org,
tglx@linutronix.de
In-Reply-To: <20120321170003.GA5533@netboy.at.omicron.at>
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, March 21, 2012 10:00 AM
> To: chetan loke
> Cc: netdev@vger.kernel.org; e1000-devel@lists.sourceforge.net; Keller, Jacob
> E; Kirsher, Jeffrey T; Ronciak, John; john.stultz@linaro.org;
> tglx@linutronix.de
> Subject: Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the
> timecompare method
>
> Can you please explain how using a seqlock could help here?
>
> Thanks,
> Richard
My understanding of the seqlock, is that it prevents starvation of the hwtstamp calls in the rx and tx routines if/when a user hammers the gettime ioctl due to bad software design where 100+ apps are wanting direct access to the PHC.
- Jake
^ permalink raw reply
* Re: use-after-free in usbnet
From: Alan Stern @ 2012-03-21 17:30 UTC (permalink / raw)
To: Ming Lei
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Fedora Kernel Team, Dave Jones
In-Reply-To: <CACVXFVOVjnWjqpKxbU98DAyUC_OSb8ZL-3WcyYuFXgPJn5UyuA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, 22 Mar 2012, Ming Lei wrote:
> 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);
> }
Yes, that looks about right. But I'm not familiar with the details of
usbnet.
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
* e1000e: Avoid wrong check on TX hang
From: Joakim Tjernlund @ 2012-03-21 17:20 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: netdev
I think commit 09357b00255c233705b1cf6d76a8d147340545b8(e1000e: Avoid wrong check on TX hang)
needs to be applied in 3.2.x (and 3.0.x) too.
We have seen this on routers(we got 3):
e1000e 0000:11:00.0: eth9: Detected Hardware Unit Hang:
TDH <a4>
TDT <a7>
next_to_use <a7>
next_to_clean <a4>
buffer_info[next_to_clean]:
time_stamp <1008e849f>
next_to_watch <a4>
jiffies <1008e85d8>
next_to_watch.status <0>
MAC Status <80387>
PHY Status <792d>
PHY 1000BASE-T Status <3800>
PHY Extended Status <3000>
PCI Status <10>
Applying the above patch on 3.2.12 makes the error go away.
Jocke
^ permalink raw reply
* RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Parav.Pandit @ 2012-03-21 18:58 UTC (permalink / raw)
To: roland; +Cc: linux-rdma, netdev
In-Reply-To: <CAL1RGDVFTjsF43A9ebXwtgiuY5s1yuhFg2CRajABZ=MTeT0+Pg@mail.gmail.com>
> -----Original Message-----
> From: Roland Dreier [mailto:roland@purestorage.com]
> Sent: Wednesday, March 21, 2012 9:44 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> > +#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.
o.k. I'll change it.
>
>
> > +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?
>
From beginning its cacheline aligned. So didn't tested it without it, but yes this is considered. I'll be shifting other widely used elements before the lock so that hole is smaller.
> > + 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.
o.k. I'll change it.
^ permalink raw reply
* RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA @ 2012-03-21 19:02 UTC (permalink / raw)
To: roland-BHEL68pLQRGGvPXPguhicg
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAL1RGDVxCE--P78bk0Me5o+ekSzgBYG0UJT6y3O7cK3mUGBjuQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
I see couple of comments on rsvd words.
They were primarily not introduced for alignment. But there are other new features that we will be adding with new set of hardware and firmware updates.
I don't want to change the user-kernel interface at such stage by modifying the size of the structure.
For some features its under testing stage internally.
So once its ready rsvd will be replaced with actual element.
This will avoid abi compatibility issues between library and driver.
I'll consider alignment macro too so that compiler related byte alignment access issue also gets resolved.
Parav
> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Wednesday, March 21, 2012 9:50 PM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> 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 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA @ 2012-03-21 19:04 UTC (permalink / raw)
To: roland-BHEL68pLQRGGvPXPguhicg
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAL1RGDWnc478=ToFQEC51Usn6LLZgLen=CymFZ0C0GnRp9BAsw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Wednesday, March 21, 2012 9:56 PM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> > +/* 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...
This pack is required as it is shared with hardware and need to be of 16 bytes for 32 and 64 bit architecture. Do not wanted to take risk of different compiler versions. So keeping it packed.
>
> > +#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...
Yes, I'll change to inline function.
--
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
* locking in net_device_ops callbacks
From: Jeff Haran @ 2012-03-21 18:59 UTC (permalink / raw)
To: netdev@vger.kernel.org
Hi,
I had posted the below to the kernelnewbies email list and never got a response. I am hoping somebody on this list could provide some feedback.
Just to be clear, it's not the issue of copying the MAC address that I am asking about. That's just an example.
What I am trying to understand is, what mechanisms generally are at play to serialize access to struct net_device private data that is accessed in both process and softirq contexts? Explicit locking "seems" to be mostly absent from the driver sources I've inspected so I can't help but believe I am missing something fundamental here.
Thanks in advance,
Jeff Haran
I was hoping somebody could enlighten me on how serialization of access
to data in struct net_device and associated netdev_priv() data works in
the callback functions registered in net_device_ops. For example, I am
looking at drivers/net/e1000/e1000_main.c in my 2.6.32 based kernel
tree. My understanding is this is a reference driver of sorts, so it
should be doing the "right thing".
This driver registers a callback for the ndo_set_mac_address callback
like this (much of what I think is not significant to the question code
replaced with "..." below for brevity):
static const struct net_device_ops e1000_netdev_ops = {
...
.ndo_set_mac_address = e1000_set_mac,
...
};
static int __devinit e1000_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
struct net_device *netdev;
....
netdev = alloc_etherdev(sizeof(struct e1000_adapter));
....
netdev->netdev_ops = &e1000_netdev_ops;
...
err = register_netdev(netdev);
...
}
Most network drivers seem to follow more or less the same initialization
sequence.
When I look at the implementation of e1000_set_mac(), I see this:
static int e1000_set_mac(struct net_device *netdev, void *p)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
struct sockaddr *addr = p;
...
memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
memcpy(hw->mac_addr, addr->sa_data, netdev->addr_len);
...
return 0;
}
I can't help but notice there is no locking going on around the
memcpy()s of the passed in MAC address to the net_device dev_addr and
private data mac_addr fields.
I assume that e1000_set_mac() is typically called in process context in
response to some user space application like ifconfig or ip changing the
interface MAC address. I am also assuming that these dev_addr and
mac_addr fields are also referenced in other contexts. An Ethernet MAC
address is 6 bytes, so the memcpy()'s can't be atomic operations at
least on a 32 bit machine.
Shouldn't there have been some sort of lock taken before the memcpy()s
are executed so that other execution contexts won't see a partially
copied MAC address?
Is there some sort of lock taken higher up the call stack by the code
that calls these callback functions so that the callbacks themselves
don't have to do it explicitly?
I am writing a network device driver (modifying an existing one
actually) and am therefore trying to understand what kinds of explicit
locking my net_device_ops callbacks need to take in order to ensure
proper operation on an SMP system.
Thanks,
Jeff Haran
^ permalink raw reply
* RE: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Parav.Pandit @ 2012-03-21 19:09 UTC (permalink / raw)
To: roland; +Cc: linux-rdma, netdev
In-Reply-To: <CAL1RGDWRm7GVCBgAdSvAt72WQAP4a-1xa18Hk-rFjEfKDEHZNg@mail.gmail.com>
> -----Original Message-----
> From: Roland Dreier [mailto:roland@purestorage.com]
> Sent: Wednesday, March 21, 2012 10:04 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> > +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.
Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be returned during poll_cq() phase.
So state machine is implemented above.
^ permalink raw reply
* RE: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA @ 2012-03-21 19:10 UTC (permalink / raw)
To: roland-BHEL68pLQRGGvPXPguhicg
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAL1RGDVv6y-EOiNv-Vmd+jN1dnSPvA0YK_kamYZwmgUzRmUx=g@mail.gmail.com>
> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Wednesday, March 21, 2012 10:13 PM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> > +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?
This use_cnt can be removed from low-level hardware driver. I'll remove it.
--
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 net-next] bonding: remove entries for master_ip and vlan_ip and query devices instead
From: Andy Gospodarek @ 2012-03-21 16:25 UTC (permalink / raw)
To: David Miller; +Cc: andy, fubar, netdev, ralf.zeidler
In-Reply-To: <20120316.225533.1194931730650486577.davem@davemloft.net>
On Fri, Mar 16, 2012 at 10:55:33PM -0700, David Miller wrote:
> From: Andy Gospodarek <andy@greyhouse.net>
> Date: Fri, 16 Mar 2012 09:48:23 -0400
>
> > On Thu, Mar 15, 2012 at 9:03 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
> >>>@@ -2618,7 +2624,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
> >>> if (!bond_vlan_used(bond)) {
> >>> pr_debug("basa: empty vlan: arp_send\n");
> >>> bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
> >>>- bond->master_ip, 0);
> >>>+ bond_confirm_addr(bond->dev,
> >>>+ targets[i],
> >>>+ 0), 0);
> >>
> >> Same comment here and for the later calls to bond_confirm_addr,
> >> here putting "targets[i]" and perhaps the 0 on the previous line,
> >> although I'm less sure that it won't look funky.
> >>
> >> -J
> >>
> >
> > These we a bit tough to get looking great. What I did really seemed
> > like the best I could do and keep it to a reasonable length. If you
> > want me to just keep the length of these lines <100 characters wide, I
> > could combine them into the same line. Either way is fine with me,
> > but I really just didn't want the code to get too wide and hard to
> > read when using a standard size terminal.
>
> It seems to me that the easiest thing to do is:
>
> __be32 addr = bond_confirm_addr(bond->dev, targets[i], 0);
> bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], addr, 0);
>
> And actually this sequence is used in three places, so even better
> to put it into a helper function.
For readability it makes sense to pop this function out like you have
suggested. I'm not sure I want to make a helper function for both
calls, but I'll take a look at post an update patch.
^ permalink raw reply
* Re: locking in net_device_ops callbacks
From: Stephen Hemminger @ 2012-03-21 19:25 UTC (permalink / raw)
To: Jeff Haran; +Cc: netdev@vger.kernel.org
In-Reply-To: <471DE477E2B1F549A58F3A14A27180BE014357@HQ-EX01.bytemobile.com>
On Wed, 21 Mar 2012 18:59:30 +0000
Jeff Haran <jharan@bytemobile.com> wrote:
> Hi,
>
> I had posted the below to the kernelnewbies email list and never got a response. I am hoping somebody on this list could provide some feedback.
>
> Just to be clear, it's not the issue of copying the MAC address that I am asking about. That's just an example.
>
> What I am trying to understand is, what mechanisms generally are at play to serialize access to struct net_device private data that is accessed in both process and softirq contexts? Explicit locking "seems" to be mostly absent from the driver sources I've inspected so I can't help but believe I am missing something fundamental here.
Look at rtnl_lock() in net/core/rtnetlink.c; this is global mutex against
all changes to network device state.
^ permalink raw reply
* Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Roland Dreier @ 2012-03-21 19:31 UTC (permalink / raw)
To: Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <88B766C272F2C64B944B21AD078333151C964A63FB-/SwythR3zqxVRK6PHKByhFaTQe2KTcn/@public.gmane.org>
On Wed, Mar 21, 2012 at 12:09 PM, <Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be returned during poll_cq() phase.
> So state machine is implemented above.
Couldn't you just write
if (ib_modify_qp_is_ok(...)) {
if (new_state == OCRDMA_QPS_ERR)
ocrdma_flush_qp(qp);
} else {
status = -EINVAL;
}
and save about 100 lines of code?
- R.
--
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 19:33 UTC (permalink / raw)
To: Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <88B766C272F2C64B944B21AD078333151C964A63F1-/SwythR3zqxVRK6PHKByhFaTQe2KTcn/@public.gmane.org>
On Wed, Mar 21, 2012 at 12:04 PM, <Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
>> > +/* 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...
> This pack is required as it is shared with hardware and need to be of 16 bytes for 32 and 64 bit architecture. Do not wanted to take risk of different compiler versions. So keeping it packed.
I really think if you can't trust your compiler to lay this structure
out properly,
you have a lot of bigger problems. But whatever, it's not a big deal.
- R.
--
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: Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA @ 2012-03-21 19:46 UTC (permalink / raw)
To: roland-BHEL68pLQRGGvPXPguhicg
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAL1RGDWrwP3mY2=W42_c5wpefzqx_BnXhnwfy2fPrP=12hrBOw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Thursday, March 22, 2012 1:02 AM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> On Wed, Mar 21, 2012 at 12:09 PM, <Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> > Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be
> returned during poll_cq() phase.
> > So state machine is implemented above.
>
> Couldn't you just write
>
> if (ib_modify_qp_is_ok(...)) {
> if (new_state == OCRDMA_QPS_ERR)
> ocrdma_flush_qp(qp);
> } else {
> status = -EINVAL;
> }
>
> and save about 100 lines of code?
>
Yes, this can be done. This is one path.
Another path is async_event coming from adapter. So I still need qp_state_machine function but as you suggested, I'll remove the states and will have invoke flush_qp() on error.
> - R.
--
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: locking in net_device_ops callbacks
From: Ben Hutchings @ 2012-03-21 19:58 UTC (permalink / raw)
To: Jeff Haran; +Cc: netdev@vger.kernel.org
In-Reply-To: <471DE477E2B1F549A58F3A14A27180BE014357@HQ-EX01.bytemobile.com>
On Wed, 2012-03-21 at 18:59 +0000, Jeff Haran wrote:
> Hi,
>
> I had posted the below to the kernelnewbies email list and never got a
> response. I am hoping somebody on this list could provide some
> feedback.
>
> Just to be clear, it's not the issue of copying the MAC address that I
> am asking about. That's just an example.
>
> What I am trying to understand is, what mechanisms generally are at
> play to serialize access to struct net_device private data that is
> accessed in both process and softirq contexts? Explicit locking
> "seems" to be mostly absent from the driver sources I've inspected so
> I can't help but believe I am missing something fundamental here.
[...]
> I can't help but notice there is no locking going on around the
> memcpy()s of the passed in MAC address to the net_device dev_addr and
> private data mac_addr fields.
>
> I assume that e1000_set_mac() is typically called in process context in
> response to some user space application like ifconfig or ip changing the
> interface MAC address.
Yes. Most interface reconfiguration is done in process context, with
the (global) rtnetlink lock held. The address *lists* (additional
unicast addresses and multicast addresses) may be updated in softirq
context, and are updated with the (per-interface) addr_lock held.
> I am also assuming that these dev_addr and
> mac_addr fields are also referenced in other contexts. An Ethernet MAC
> address is 6 bytes, so the memcpy()'s can't be atomic operations at
> least on a 32 bit machine.
>
> Shouldn't there have been some sort of lock taken before the memcpy()s
> are executed so that other execution contexts won't see a partially
> copied MAC address?
As I think you've recognised, the MAC address may still be read when
constructing packet headers. eth_mac_addr() fails if the interface is
running, which should avoid this problem. However some other
implementations of ndo_set_mac_address don't check that. Maybe they
should - or else that check should be done in the caller,
dev_set_mac_address().
> Is there some sort of lock taken higher up the call stack by the code
> that calls these callback functions so that the callbacks themselves
> don't have to do it explicitly?
In most cases, yes.
> I am writing a network device driver (modifying an existing one
> actually) and am therefore trying to understand what kinds of explicit
> locking my net_device_ops callbacks need to take in order to ensure
> proper operation on an SMP system.
I would like to point you to documentation, but unfortunately I can't
see an up-to-date description.
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: xtables question
From: Sri Ram Vemulpali @ 2012-03-21 20:20 UTC (permalink / raw)
To: linux-kernel-mail, linux-netdev
In-Reply-To: <CALyraeOX0nA6kVh+jtm1C8LnbE8mDvL+jcKcbXxYNkdXfm1REw@mail.gmail.com>
Sending the e-mail again
On Tue, Mar 20, 2012 at 6:34 PM, Sri Ram Vemulpali
<sri.ram.gmu06@gmail.com> wrote:
> Hi guys,
>
> I am trying to implement the rules in kernel using xtables. I have
> very basic knowledge of xtables framework. Is there any resource where
> there is coherent explanation of subject. The problem is there is no
> proper explanation of each and every field of structures used in
> xtables. I know the netfilter basic framework. If anyone can point me
> to clean explanation and also how to send rules from user space to
> kernel. Do not ask me to search the google, I learned the basic
> framework from wikipedia references.
>
> Hope someone can provide resource.
>
> Thanks in advance.
>
> --
> Regards,
> Sri.
--
Regards,
Sri.
^ permalink raw reply
* Re: [PATCH] sky2: override for PCI legacy power management
From: Bjorn Helgaas @ 2012-03-21 20:22 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Knut Petersen, David S. Miller, Linus Torvalds, arekm, Jared,
dilieto, linux-kernel, netdev
In-Reply-To: <20120321083205.360a7a3b@nehalam.linuxnetplumber.net>
On Wed, Mar 21, 2012 at 9:32 AM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> 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>
Is there a problem report URL you can include here?
It looks like this requires a user to figure out that he might be
suffering from this problem, then use this module parameter to work
around it. How would a user figure that out? Can we do it
automatically to save him the trouble?
> ---
> 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);
> --
> 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: locking in net_device_ops callbacks
From: Jeff Haran @ 2012-03-21 20:38 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev@vger.kernel.org
In-Reply-To: <20120321122503.25838566@nehalam.linuxnetplumber.net>
> -----Original Message-----
> From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> Sent: Wednesday, March 21, 2012 12:25 PM
> To: Jeff Haran
> Cc: netdev@vger.kernel.org
> Subject: Re: locking in net_device_ops callbacks
>
> On Wed, 21 Mar 2012 18:59:30 +0000
> Jeff Haran <jharan@bytemobile.com> wrote:
>
> > Hi,
> >
> > I had posted the below to the kernelnewbies email list and never got a
> response. I am hoping somebody on this list could provide some feedback.
> >
> > Just to be clear, it's not the issue of copying the MAC address that I am
> asking about. That's just an example.
> >
> > What I am trying to understand is, what mechanisms generally are at play
> to serialize access to struct net_device private data that is accessed in both
> process and softirq contexts? Explicit locking "seems" to be mostly absent
> from the driver sources I've inspected so I can't help but believe I am missing
> something fundamental here.
>
> Look at rtnl_lock() in net/core/rtnetlink.c; this is global mutex against
> all changes to network device state.
Steve,
Thanks for the response.
rtnl_lock() does this:
void rtnl_lock(void)
{
mutex_lock(&rtnl_mutex);
}
I can see where that would serialize process context access, but as I understand it much of the networking stack runs in soft IRQ context. Soft IRQs can't take mutexes, can they?
It seems to me that some other serialization mechanism should be in place to serialize soft IRQ context access to instances of struct net_device.
Jeff Haran
^ permalink raw reply
* Re: [PATCH] sky2: override for PCI legacy power management
From: Stephen Hemminger @ 2012-03-21 20:47 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Knut Petersen, David S. Miller, Linus Torvalds, arekm, Jared,
dilieto, linux-kernel, netdev
In-Reply-To: <CAErSpo67PEaZ6JuMUntWLfR8SUpQgZERD_cKV6rWd=FF0tZ6DQ@mail.gmail.com>
On Wed, 21 Mar 2012 14:22:01 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Mar 21, 2012 at 9:32 AM, Stephen Hemminger
> <shemminger@vyatta.com> wrote:
> > 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>
>
> Is there a problem report URL you can include here?
>
> It looks like this requires a user to figure out that he might be
> suffering from this problem, then use this module parameter to work
> around it. How would a user figure that out? Can we do it
> automatically to save him the trouble?
I am not a power management expert. Looks like a BIOS issue where the
BIOS has configured the device to disable power management but the
user wants to override that value. There is no method to determine
when the BIOS is broken versus when the BIOS setting is correct and
we should follow what it says.
^ 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