* Re: [PATCH V2] CAN: Add Flexcan CAN controller driver
From: Sascha Hauer @ 2009-07-28 14:24 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Socketcan-core, Linux Netdev List
In-Reply-To: <4A6F073A.1050909@hartkopp.net>
On Tue, Jul 28, 2009 at 04:12:10PM +0200, Oliver Hartkopp wrote:
> Oliver Hartkopp wrote:
> > Sascha Hauer wrote:
>
> >>>> +
> >>>> + if (frame->can_id & CAN_RTR_FLAG)
> >>>> + dlc |= MB_CNT_RTR;
> >>>> +
> >>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
> >>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
>
> Are you sure, that this is correct?
Yes, I am sure, at least on my hardware.
>
> Indeed i wonder, if it would make sense to skip the entire struct flexcan_mb
> approach and fiddle byte-by-byte inside the registers ...
You'll have to to 32 bit accesses to get it right on little and big
endian.
what we can do is:
writel(can_data[0] << 24 | can_data[1] << 16 | can_data[2] << 8 | can_data[3],
msg_buf + 0x8);
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH V2] CAN: Add Flexcan CAN controller driver
From: Sascha Hauer @ 2009-07-28 14:18 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Socketcan-core, Linux Netdev List
In-Reply-To: <4A6F0238.6050605@hartkopp.net>
On Tue, Jul 28, 2009 at 03:50:48PM +0200, Oliver Hartkopp wrote:
> Sascha Hauer wrote:
> > Hi Oliver,
> >
> > On Tue, Jul 28, 2009 at 03:21:40PM +0200, Oliver Hartkopp wrote:
> >> Sascha Hauer wrote:
> >>> Hi,
> >>>
> >>> Here is the second version of the flexcan driver.
> >> Hi Sascha,
> >>
> >> some more points i forgot to mention, sorry ...
> >>
> >>
> >>> +/* Structure of the message buffer */
> >>> +struct flexcan_mb {
> >>> + u32 can_dlc;
> >>> + u32 can_id;
> >>> + u32 data[2];
> >>> +};
> >> This looks really hackish and does not reflect the structure of a flexcan
> >> message buffer! The data is 'u8' and the name of 'dlc' for the
> >> description/flag register is bad.
> >>
> >
> > see below..
>
> Especially can_dlc, can_id and data[] are known from struct can_frame which
> really can confuse here ...
>
> >
> >>> +
> >>> +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >>> +{
> >>> + struct can_frame *frame = (struct can_frame *)skb->data;
> >>> + struct flexcan_priv *priv = netdev_priv(dev);
> >>> + struct flexcan_regs __iomem *regs = priv->base;
> >>> + u32 can_id;
> >>> + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
> >> Naming this variable 'dlc' does not hit the point. See below.
> >>
> >>> + u32 *can_data;
> >> Really this needs to be fixed up by defining a proper mailbox struct.
> >>
> >>
> >>> +
> >>> + netif_stop_queue(dev);
> >>> +
> >>> + if (frame->can_id & CAN_EFF_FLAG) {
> >>> + can_id = frame->can_id & MB_ID_EXT;
> >> Please use CAN_EFF_MASK here.
> >
> > I used MX_ID_EXT intentionally because it it flexcan specific and just
> > happens to be the same as CAN_EFF_MASK. I can change it if you like.
>
> Yes, i've seen that. I would tend to use CAN_EFF_MASK here as you apply it on
> frame->can_id.
>
> When you get it from the controller MB_ID_EXT_MASK would be the better one.
>
> >
> >>
> >>> + dlc |= MB_CNT_IDE | MB_CNT_SRR;
> >>> + } else {
> >>> + can_id = (frame->can_id & CAN_SFF_MASK) << 18;
> >>> + }
> >> Just nitpicking for Kernel coding style:
> >> remove the last '{' and '}' pair.
> >
> > No, Documentation/CondingStyle suggests that if one branch needs braces
> > the other branch should use them, too.
>
> Sorry. Didn't know that.
>
> >
> >>> +
> >>> + if (frame->can_id & CAN_RTR_FLAG)
> >>> + dlc |= MB_CNT_RTR;
> >>> +
> >>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
> >>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
> >>> +
> >>> + can_data = (u32 *)frame->data;
> >>> + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]);
> >>> + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]);
> >> IMHO it is not really transparent, that this is a correct handling to copy the
> >> can_frame.data[] on all architectures. I bet creating a for-statement
> >> regarding the dlc is not slower and makes really clear, what's going on here.
> >
> > This is indeed a problem here. The original Coldfire code I used as a
> > template used a loop around unsigned char * which did the wrong thing
> > for me.
>
> This could be a good starting point for an investigation ;-)
>
> > So yes, this is not generic here, but I have no idea how the
> > generic code looks like. As Coldfire is big endian this doesn't seem
> > that wrong.
>
> I would try to define a proper flexcan_mb struct like
>
> struct flexcan_mb {
> u8 code;
> u8 ctrl;
> u16 timestamp;
> u32 id;
> u8 data[8];
> }
I can't properly define it like this because I have a little endian
system and u8 code is on offset 3 on my hardware. I just checked it
because I always get confused with endian problems ;)
I hope you're not suggesting me to do something like
#ifdef __LITTLE_ENDIAN
struct flexcan_mb {
u16 timestamp;
u8 ctrl;
u8 code;
u32 id;
u8 data[8];
}
#else
struct flexcan_mb {
u8 code;
u8 ctrl;
u16 timestamp;
u32 id;
u8 data[8];
}
#endif
(which would still require endian specific handling for the actual CAN
data)
>
> And then see how it looks like ;-)
Well, I would have to do a le/be conversion manually whereas cpu_to_be32
*should* do the right thing. I don't have any ColdFire hardware to test
though.
The more I think about it the more I think that my original code does
the right thing(tm)
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH V2] CAN: Add Flexcan CAN controller driver
From: Oliver Hartkopp @ 2009-07-28 14:12 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Socketcan-core, Linux Netdev List
In-Reply-To: <4A6F0238.6050605@hartkopp.net>
Oliver Hartkopp wrote:
> Sascha Hauer wrote:
>>>> +
>>>> + if (frame->can_id & CAN_RTR_FLAG)
>>>> + dlc |= MB_CNT_RTR;
>>>> +
>>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
>>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
Are you sure, that this is correct?
Indeed i wonder, if it would make sense to skip the entire struct flexcan_mb
approach and fiddle byte-by-byte inside the registers ...
Regards,
Oliver
^ permalink raw reply
* Re: [PATCH V2] CAN: Add Flexcan CAN controller driver
From: Oliver Hartkopp @ 2009-07-28 13:50 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Socketcan-core, Linux Netdev List
In-Reply-To: <20090728133719.GU2714@pengutronix.de>
Sascha Hauer wrote:
> Hi Oliver,
>
> On Tue, Jul 28, 2009 at 03:21:40PM +0200, Oliver Hartkopp wrote:
>> Sascha Hauer wrote:
>>> Hi,
>>>
>>> Here is the second version of the flexcan driver.
>> Hi Sascha,
>>
>> some more points i forgot to mention, sorry ...
>>
>>
>>> +/* Structure of the message buffer */
>>> +struct flexcan_mb {
>>> + u32 can_dlc;
>>> + u32 can_id;
>>> + u32 data[2];
>>> +};
>> This looks really hackish and does not reflect the structure of a flexcan
>> message buffer! The data is 'u8' and the name of 'dlc' for the
>> description/flag register is bad.
>>
>
> see below..
Especially can_dlc, can_id and data[] are known from struct can_frame which
really can confuse here ...
>
>>> +
>>> +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>> +{
>>> + struct can_frame *frame = (struct can_frame *)skb->data;
>>> + struct flexcan_priv *priv = netdev_priv(dev);
>>> + struct flexcan_regs __iomem *regs = priv->base;
>>> + u32 can_id;
>>> + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
>> Naming this variable 'dlc' does not hit the point. See below.
>>
>>> + u32 *can_data;
>> Really this needs to be fixed up by defining a proper mailbox struct.
>>
>>
>>> +
>>> + netif_stop_queue(dev);
>>> +
>>> + if (frame->can_id & CAN_EFF_FLAG) {
>>> + can_id = frame->can_id & MB_ID_EXT;
>> Please use CAN_EFF_MASK here.
>
> I used MX_ID_EXT intentionally because it it flexcan specific and just
> happens to be the same as CAN_EFF_MASK. I can change it if you like.
Yes, i've seen that. I would tend to use CAN_EFF_MASK here as you apply it on
frame->can_id.
When you get it from the controller MB_ID_EXT_MASK would be the better one.
>
>>
>>> + dlc |= MB_CNT_IDE | MB_CNT_SRR;
>>> + } else {
>>> + can_id = (frame->can_id & CAN_SFF_MASK) << 18;
>>> + }
>> Just nitpicking for Kernel coding style:
>> remove the last '{' and '}' pair.
>
> No, Documentation/CondingStyle suggests that if one branch needs braces
> the other branch should use them, too.
Sorry. Didn't know that.
>
>>> +
>>> + if (frame->can_id & CAN_RTR_FLAG)
>>> + dlc |= MB_CNT_RTR;
>>> +
>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
>>> +
>>> + can_data = (u32 *)frame->data;
>>> + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]);
>>> + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]);
>> IMHO it is not really transparent, that this is a correct handling to copy the
>> can_frame.data[] on all architectures. I bet creating a for-statement
>> regarding the dlc is not slower and makes really clear, what's going on here.
>
> This is indeed a problem here. The original Coldfire code I used as a
> template used a loop around unsigned char * which did the wrong thing
> for me.
This could be a good starting point for an investigation ;-)
> So yes, this is not generic here, but I have no idea how the
> generic code looks like. As Coldfire is big endian this doesn't seem
> that wrong.
I would try to define a proper flexcan_mb struct like
struct flexcan_mb {
u8 code;
u8 ctrl;
u16 timestamp;
u32 id;
u8 data[8];
}
And then see how it looks like ;-)
Regards,
Oliver
^ permalink raw reply
* [PATCH] pppol2tp: calls unregister_pernet_gen_device() at unload time
From: Eric Dumazet @ 2009-07-28 13:47 UTC (permalink / raw)
To: David S. Miller
Cc: Pavel Emelyanov, Igor M Podlesny, Andrew Morton, netdev,
Cyrill Gorcunov
In-Reply-To: <4A6EFB81.4090105@gmail.com>
Eric Dumazet a écrit :
> Seems drivers/net/pppol2tp.c is a suspect...
>
> It uses register_pernet_gen_device() from pppol2tp_init()
> but doesnt call unregister_pernet_gen_device()
OK patch seems really easy...
This bug was added in commit 4e9fb8016a351b5b9da7fea32bcfdbc9d836e421
net: pppol2tp - introduce net-namespace functionality
So this is a stable candidate I guess ?
Thank you
[PATCH] pppol2tp: calls unregister_pernet_gen_device() at unload time
Failure to call unregister_pernet_gen_device() can exhaust memory
if module is loaded/unloaded many times.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c
index e7935d0..e0f9219 100644
--- a/drivers/net/pppol2tp.c
+++ b/drivers/net/pppol2tp.c
@@ -2680,6 +2680,7 @@ out_unregister_pppol2tp_proto:
static void __exit pppol2tp_exit(void)
{
unregister_pppox_proto(PX_PROTO_OL2TP);
+ unregister_pernet_gen_device(pppol2tp_net_id, &pppol2tp_net_ops);
proto_unregister(&pppol2tp_sk_proto);
}
^ permalink raw reply related
* Re: [PATCH V2] CAN: Add Flexcan CAN controller driver
From: Sascha Hauer @ 2009-07-28 13:37 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Socketcan-core, Linux Netdev List
In-Reply-To: <4A6EFB64.8070804@hartkopp.net>
Hi Oliver,
On Tue, Jul 28, 2009 at 03:21:40PM +0200, Oliver Hartkopp wrote:
> Sascha Hauer wrote:
> > Hi,
> >
> > Here is the second version of the flexcan driver.
>
> Hi Sascha,
>
> some more points i forgot to mention, sorry ...
>
>
> > +/* Structure of the message buffer */
> > +struct flexcan_mb {
> > + u32 can_dlc;
> > + u32 can_id;
> > + u32 data[2];
> > +};
>
> This looks really hackish and does not reflect the structure of a flexcan
> message buffer! The data is 'u8' and the name of 'dlc' for the
> description/flag register is bad.
>
see below..
>
> > +
> > +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > + struct can_frame *frame = (struct can_frame *)skb->data;
> > + struct flexcan_priv *priv = netdev_priv(dev);
> > + struct flexcan_regs __iomem *regs = priv->base;
>
> > + u32 can_id;
> > + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
>
> Naming this variable 'dlc' does not hit the point. See below.
>
> > + u32 *can_data;
>
> Really this needs to be fixed up by defining a proper mailbox struct.
>
>
> > +
> > + netif_stop_queue(dev);
> > +
> > + if (frame->can_id & CAN_EFF_FLAG) {
> > + can_id = frame->can_id & MB_ID_EXT;
>
> Please use CAN_EFF_MASK here.
I used MX_ID_EXT intentionally because it it flexcan specific and just
happens to be the same as CAN_EFF_MASK. I can change it if you like.
>
>
> > + dlc |= MB_CNT_IDE | MB_CNT_SRR;
> > + } else {
> > + can_id = (frame->can_id & CAN_SFF_MASK) << 18;
> > + }
>
> Just nitpicking for Kernel coding style:
> remove the last '{' and '}' pair.
No, Documentation/CondingStyle suggests that if one branch needs braces
the other branch should use them, too.
>
> > +
> > + if (frame->can_id & CAN_RTR_FLAG)
> > + dlc |= MB_CNT_RTR;
> > +
> > + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
> > + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
> > +
> > + can_data = (u32 *)frame->data;
> > + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]);
> > + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]);
>
> IMHO it is not really transparent, that this is a correct handling to copy the
> can_frame.data[] on all architectures. I bet creating a for-statement
> regarding the dlc is not slower and makes really clear, what's going on here.
This is indeed a problem here. The original Coldfire code I used as a
template used a loop around unsigned char * which did the wrong thing
for me. So yes, this is not generic here, but I have no idea how the
generic code looks like. As Coldfire is big endian this doesn't seem
that wrong.
>
> > +
> > + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
> > +
> > + kfree_skb(skb);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static void flexcan_rx_frame(struct net_device *ndev,
> > + struct flexcan_mb __iomem *mb)
> > +{
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct sk_buff *skb;
> > + struct can_frame *frame;
> > + int ctrl = readl(&mb->can_dlc);
>
> 'ctrl' is much better than 'dlc' naming above :-)
ok, will change it above.
>
> > + int length = (ctrl >> 16) & 0x0f;
>
> is probably not enough ... use %9 or an additional check.
ok, I'll add a sanity check.
>
> > + u32 id;
> > +
> > + skb = dev_alloc_skb(sizeof(struct can_frame));
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return;
> > + }
> > +
> > + frame = (struct can_frame *)skb_put(skb,
> > + sizeof(struct can_frame));
> > +
> > + frame->can_dlc = length;
> > + id = readl(&mb->can_id) & MB_ID_EXT;
>
> like above use CAN_EFF_MASK
>
> or at least rename MB_ID_EXT to MB_ID_EXT_MASK
>
> > +
> > + if (ctrl & MB_CNT_IDE) {
> > + frame->can_id = id & CAN_EFF_MASK;
> > + frame->can_id |= CAN_EFF_FLAG;
> > + if (ctrl & MB_CNT_RTR)
> > + frame->can_id |= CAN_RTR_FLAG;
> > + } else {
> > + frame->can_id = id >> 18;
> > + if (ctrl & MB_CNT_RTR)
> > + frame->can_id |= CAN_RTR_FLAG;
> > + }
> > +
> > + *(u32 *)frame->data = be32_to_cpu(readl(&mb->data[0]));
> > + *((u32 *)frame->data + 1) = be32_to_cpu(readl(&mb->data[1]));
>
> Same as above.
>
> Please write readable an maintainable code and let the compiler do his job.
>
> Thanks,
> Oliver
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH] ppp: fix lost fragments in ppp_mp_explode()
From: Ben McKeegan @ 2009-07-28 13:35 UTC (permalink / raw)
To: davem, paulus; +Cc: gabriele.paoloni, netdev, linux-ppp
This patch fixes the corner cases where the sum of MTU of the free
channels (adjusted for fragmentation overheads) is less than the MTU of
PPP link. There are at least 3 situations where this case might arise:
- some of the channels are busy
- the multilink session is running in a degraded state (i.e. with less
than its full complement of active channels)
- by design, where multilink protocol is being used to artificially
increase the effective link MTU of a single link.
Without this patch, at most 1 fragment is ever sent per free channel for
a given PPP frame and any remaining part of the PPP frame that does not
fit into those fragments is silently discarded.
This patch restores the original behaviour which was broken by commit
9c705260feea6ae329bc6b6d5f6d2ef0227eda0a 'ppp:ppp_mp_explode()
redesign'. Once all 'free' channels have been given a fragment, an
additional fragment is queued to each available channel in turn, as many
times as necessary, until the entire PPP frame has been consumed.
Signed-off-by: Ben McKeegan <ben@netservers.co.uk>
---
diff -uprN linux-2.6.31-rc4/drivers/net/ppp_generic.c
linux-2.6.31-rc4-multilink-fix/drivers/net/ppp_generic.c
--- linux-2.6.31-rc4/drivers/net/ppp_generic.c 2009-07-23
03:32:59.000000000 +0100
+++ linux-2.6.31-rc4-multilink-fix/drivers/net/ppp_generic.c 2009-07-28
13:54:49.000000000 +0100
@@ -1384,7 +1384,7 @@ static int ppp_mp_explode(struct ppp *pp
/* create a fragment for each channel */
bits = B;
- while (nfree > 0 && len > 0) {
+ while (len > 0) {
list = list->next;
if (list == &ppp->channels) {
i = 0;
@@ -1431,29 +1431,31 @@ static int ppp_mp_explode(struct ppp *pp
*otherwise divide it according to the speed
*of the channel we are going to transmit on
*/
- if (pch->speed == 0) {
- flen = totlen/nfree ;
- if (nbigger > 0) {
- flen++;
- nbigger--;
- }
- } else {
- flen = (((totfree - nzero)*(totlen + hdrlen*totfree)) /
- ((totspeed*totfree)/pch->speed)) - hdrlen;
- if (nbigger > 0) {
- flen += ((totfree - nzero)*pch->speed)/totspeed;
- nbigger -= ((totfree - nzero)*pch->speed)/
+ if (nfree > 0) {
+ if (pch->speed == 0) {
+ flen = totlen/nfree ;
+ if (nbigger > 0) {
+ flen++;
+ nbigger--;
+ }
+ } else {
+ flen = (((totfree - nzero)*(totlen + hdrlen*totfree)) /
+ ((totspeed*totfree)/pch->speed)) - hdrlen;
+ if (nbigger > 0) {
+ flen += ((totfree - nzero)*pch->speed)/totspeed;
+ nbigger -= ((totfree - nzero)*pch->speed)/
totspeed;
+ }
}
+ nfree--;
}
- nfree--;
/*
*check if we are on the last channel or
*we exceded the lenght of the data to
*fragment
*/
- if ((nfree == 0) || (flen > len))
+ if ((nfree <= 0) || (flen > len))
flen = len;
/*
*it is not worth to tx on slow channels:
@@ -1467,7 +1469,7 @@ static int ppp_mp_explode(struct ppp *pp
continue;
}
- mtu = pch->chan->mtu + 2 - hdrlen;
+ mtu = pch->chan->mtu - hdrlen;
if (mtu < 4)
mtu = 4;
if (flen > mtu)
^ permalink raw reply
* Re: [PATCH] net: net_assign_generic() fix
From: Eric Dumazet @ 2009-07-28 13:22 UTC (permalink / raw)
To: Pavel Emelyanov
Cc: Igor M Podlesny, Andrew Morton, netdev, Paul E. McKenney,
David S. Miller
In-Reply-To: <4A6EFA35.3060309@gmail.com>
Eric Dumazet a écrit :
> Pavel Emelyanov a écrit :
>>> Hmm...
>>>
>>> Real bug may be fixed by followed patch ? (yet untested, sorry...)
>>>
>>> [PATCH] net: net_assign_generic() fix
>>>
>>> memcpy() should take into account size of pointers,
>>> not only number of pointers to copy.
>>>
>>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Acked-by: Pavel Emelyanov <xemul@openvz.org>
>
> Thanks.
>
> Still this doesnt explain the crash, because initial number of pointers is 13
> (INITIAL_NET_GEN_PTRS)
>
> We probably never realloc this array, unless a module forgets to
> unregister_pernet_gen_device() and we load/unload it many times ?
>
Seems drivers/net/pppol2tp.c is a suspect...
It uses register_pernet_gen_device() from pppol2tp_init()
but doesnt call unregister_pernet_gen_device()
^ permalink raw reply
* Re: [PATCH V2] CAN: Add Flexcan CAN controller driver
From: Oliver Hartkopp @ 2009-07-28 13:21 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Socketcan-core, Linux Netdev List
In-Reply-To: <20090728120624.GS2714@pengutronix.de>
Sascha Hauer wrote:
> Hi,
>
> Here is the second version of the flexcan driver.
Hi Sascha,
some more points i forgot to mention, sorry ...
> +/* Structure of the message buffer */
> +struct flexcan_mb {
> + u32 can_dlc;
> + u32 can_id;
> + u32 data[2];
> +};
This looks really hackish and does not reflect the structure of a flexcan
message buffer! The data is 'u8' and the name of 'dlc' for the
description/flag register is bad.
> +
> +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct can_frame *frame = (struct can_frame *)skb->data;
> + struct flexcan_priv *priv = netdev_priv(dev);
> + struct flexcan_regs __iomem *regs = priv->base;
> + u32 can_id;
> + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
Naming this variable 'dlc' does not hit the point. See below.
> + u32 *can_data;
Really this needs to be fixed up by defining a proper mailbox struct.
> +
> + netif_stop_queue(dev);
> +
> + if (frame->can_id & CAN_EFF_FLAG) {
> + can_id = frame->can_id & MB_ID_EXT;
Please use CAN_EFF_MASK here.
> + dlc |= MB_CNT_IDE | MB_CNT_SRR;
> + } else {
> + can_id = (frame->can_id & CAN_SFF_MASK) << 18;
> + }
Just nitpicking for Kernel coding style:
remove the last '{' and '}' pair.
> +
> + if (frame->can_id & CAN_RTR_FLAG)
> + dlc |= MB_CNT_RTR;
> +
> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
> +
> + can_data = (u32 *)frame->data;
> + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]);
> + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]);
IMHO it is not really transparent, that this is a correct handling to copy the
can_frame.data[] on all architectures. I bet creating a for-statement
regarding the dlc is not slower and makes really clear, what's going on here.
> +
> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
> +
> + kfree_skb(skb);
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static void flexcan_rx_frame(struct net_device *ndev,
> + struct flexcan_mb __iomem *mb)
> +{
> + struct net_device_stats *stats = &ndev->stats;
> + struct sk_buff *skb;
> + struct can_frame *frame;
> + int ctrl = readl(&mb->can_dlc);
'ctrl' is much better than 'dlc' naming above :-)
> + int length = (ctrl >> 16) & 0x0f;
is probably not enough ... use %9 or an additional check.
> + u32 id;
> +
> + skb = dev_alloc_skb(sizeof(struct can_frame));
> + if (!skb) {
> + stats->rx_dropped++;
> + return;
> + }
> +
> + frame = (struct can_frame *)skb_put(skb,
> + sizeof(struct can_frame));
> +
> + frame->can_dlc = length;
> + id = readl(&mb->can_id) & MB_ID_EXT;
like above use CAN_EFF_MASK
or at least rename MB_ID_EXT to MB_ID_EXT_MASK
> +
> + if (ctrl & MB_CNT_IDE) {
> + frame->can_id = id & CAN_EFF_MASK;
> + frame->can_id |= CAN_EFF_FLAG;
> + if (ctrl & MB_CNT_RTR)
> + frame->can_id |= CAN_RTR_FLAG;
> + } else {
> + frame->can_id = id >> 18;
> + if (ctrl & MB_CNT_RTR)
> + frame->can_id |= CAN_RTR_FLAG;
> + }
> +
> + *(u32 *)frame->data = be32_to_cpu(readl(&mb->data[0]));
> + *((u32 *)frame->data + 1) = be32_to_cpu(readl(&mb->data[1]));
Same as above.
Please write readable an maintainable code and let the compiler do his job.
Thanks,
Oliver
^ permalink raw reply
* Re: [PATCH] net: net_assign_generic() fix
From: Eric Dumazet @ 2009-07-28 13:16 UTC (permalink / raw)
To: Pavel Emelyanov
Cc: Igor M Podlesny, Andrew Morton, bugzilla-daemon, bugme-daemon,
netdev, Paul E. McKenney, David S. Miller
In-Reply-To: <4A6EF705.6070403@openvz.org>
Pavel Emelyanov a écrit :
>> Hmm...
>>
>> Real bug may be fixed by followed patch ? (yet untested, sorry...)
>>
>> [PATCH] net: net_assign_generic() fix
>>
>> memcpy() should take into account size of pointers,
>> not only number of pointers to copy.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> Acked-by: Pavel Emelyanov <xemul@openvz.org>
Thanks.
Still this doesnt explain the crash, because initial number of pointers is 13
(INITIAL_NET_GEN_PTRS)
We probably never realloc this array, unless a module forgets to
unregister_pernet_gen_device() and we load/unload it many times ?
^ permalink raw reply
* Re: [PATCH] net: net_assign_generic() fix
From: Pavel Emelyanov @ 2009-07-28 13:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: Igor M Podlesny, Andrew Morton, bugzilla-daemon, bugme-daemon,
netdev, Paul E. McKenney, David S. Miller
In-Reply-To: <4A6EF0BF.2050801@gmail.com>
> Hmm...
>
> Real bug may be fixed by followed patch ? (yet untested, sorry...)
>
> [PATCH] net: net_assign_generic() fix
>
> memcpy() should take into account size of pointers,
> not only number of pointers to copy.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Pavel Emelyanov <xemul@openvz.org>
> ---
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index b7292a2..1972830 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -488,7 +488,7 @@ int net_assign_generic(struct net *net, int id, void *data)
> */
>
> ng->len = id;
> - memcpy(&ng->ptr, &old_ng->ptr, old_ng->len);
> + memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*));
>
> rcu_assign_pointer(net->gen, ng);
> call_rcu(&old_ng->rcu, net_generic_release);
>
>
^ permalink raw reply
* Re: [Bug #13838] kernel BUG at include/net/netns/generic.h:41!
From: Eric Dumazet @ 2009-07-28 12:46 UTC (permalink / raw)
To: Luca Tettamanti
Cc: Américo Wang, Rafael J. Wysocki, Linux Kernel Mailing List,
Kernel Testers List, netdev, herbert, Alexey Dobriyan
In-Reply-To: <68676e00907280130tbbf5672x83182641b0c977b3@mail.gmail.com>
Luca Tettamanti a écrit :
> On Mon, Jul 27, 2009 at 7:29 AM, Américo Wang<xiyou.wangcong@gmail.com> wrote:
>> On Mon, Jul 27, 2009 at 4:28 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
>>> This message has been generated automatically as a part of a report
>>> of recent regressions.
>>>
>>> The following bug entry is on the current list of known regressions
>>> from 2.6.30. Please verify if it still should be listed and let me know
>>> (either way).
>>>
>>>
>>> Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=13838
>>> Subject : kernel BUG at include/net/netns/generic.h:41!
>>> Submitter : Luca Tettamanti <kronos.it@gmail.com>
>>> Date : 2009-07-20 15:27 (7 days old)
>>> References : http://lkml.org/lkml/2009/7/20/105
>> Hmm, it looks like that 'pfkey_net_id' is still zero after ipsec_pfkey_init()...
>>
>> Add Herbert and net-dev into Cc.
>>
>> Luca, would you mind to provide your .config and the steps to reproduce this?
>
> I cannot reproduce it anymore with the current git kernel. The BUG was
> triggered by racoon at startup.
> Should I go back to an older kernel to investigate or can we consider
> it "fixed"?
>
This should be fixed by following patch (submitted for bug 13760, but should apply)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index b7292a2..1972830 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -488,7 +488,7 @@ int net_assign_generic(struct net *net, int id, void *data)
*/
ng->len = id;
- memcpy(&ng->ptr, &old_ng->ptr, old_ng->len);
+ memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void *));
rcu_assign_pointer(net->gen, ng);
call_rcu(&old_ng->rcu, net_generic_release);
^ permalink raw reply related
* [PATCH] smsc95xx: remove EEPROM loaded check
From: Steve Glendinning @ 2009-07-28 12:37 UTC (permalink / raw)
To: netdev; +Cc: Ian Saturley, David Miller, Steve Glendinning
The eeprom read & write commands currently check the E2P_CMD_LOADED_ bit is
set before allowing any operations. This prevents any reading or writing
unless a correctly programmed EEPROM is installed.
This patch removes the check, so it is possible to program blank EEPROMS
via ethtool.
Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com>
---
drivers/net/usb/smsc95xx.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index fe04589..09bd635 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -220,11 +220,6 @@ static int smsc95xx_eeprom_confirm_not_busy(struct usbnet *dev)
do {
smsc95xx_read_reg(dev, E2P_CMD, &val);
- if (!(val & E2P_CMD_LOADED_)) {
- devwarn(dev, "No EEPROM present");
- return -EIO;
- }
-
if (!(val & E2P_CMD_BUSY_))
return 0;
--
1.6.2.5
^ permalink raw reply related
* [PATCH] net: net_assign_generic() fix
From: Eric Dumazet @ 2009-07-28 12:36 UTC (permalink / raw)
To: Pavel Emelyanov
Cc: Igor M Podlesny, Andrew Morton, bugzilla-daemon, bugme-daemon,
netdev, Paul E. McKenney, David S. Miller
In-Reply-To: <4A6EEF69.1050001@cosmosbay.com>
Eric Dumazet a écrit :
> Pavel Emelyanov a écrit :
>> Eric Dumazet wrote:
>>> Igor M Podlesny a écrit :
>>>> [...]
>>>>> Could have been a problem in net core, perhaps.
>>>>>
>>>>> Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.
>>>>>
>>>>> It would help if we could see that trace, please. A digital photo
>>>>> would suit.
>>>> Here it is:
>>>>
>>>> http://bugzilla.kernel.org/attachment.cgi?id=22516
>>>>
>>>> (It's 2.6.30.3)
>>>>
>>> Looking at this, I believe net_assign_generic() is not safe.
>>>
>>> Two cpus could try to expand/update the array at same time, one update could be lost.
>>>
>>> register_pernet_gen_device() has a mutex to guard against concurrent
>>> calls, but net_assign_generic() has no locking at all.
>>>
>>> I doubt this is the reason of the crash, still worth to mention it...
>>>
>>> [PATCH] net: net_assign_generic() is not SMP safe
>>>
>>> Two cpus could try to expand/update the array at same time, one update
>>> could be lost during the copy of old array.
>> How can this happen? The array is updated only during ->init routines
>> of the pernet_operations, which are called from under the net_mutex.
>>
>> Do I miss anything?
>>
>
> Oops, I missed the obvious "BUG_ON(!mutex_is_locked(&net_mutex));"
>
> Sorry for the noise and untested patch as well :)
>
Hmm...
Real bug may be fixed by followed patch ? (yet untested, sorry...)
[PATCH] net: net_assign_generic() fix
memcpy() should take into account size of pointers,
not only number of pointers to copy.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index b7292a2..1972830 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -488,7 +488,7 @@ int net_assign_generic(struct net *net, int id, void *data)
*/
ng->len = id;
- memcpy(&ng->ptr, &old_ng->ptr, old_ng->len);
+ memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*));
rcu_assign_pointer(net->gen, ng);
call_rcu(&old_ng->rcu, net_generic_release);
^ permalink raw reply related
* Re: [RFC][PATCH 5/5] libxt_ipvs: user space lib for netfilter matcher xt_ipvs
From: Hannes Eder @ 2009-07-28 12:34 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: lvs-devel, netdev, netfilter-devel, linux-kernel
In-Reply-To: <alpine.LSU.2.00.0907272037500.20984@fbirervta.pbzchgretzou.qr>
On Mon, Jul 27, 2009 at 20:40, Jan Engelhardt<jengelh@medozas.de> wrote:
>
> On Monday 2009-07-27 15:48, Hannes Eder wrote:
>>+
>>+ switch (c) {
>>+ case '0': /* --ipvs */
>>+ /* Nothing to do here. */
>
> Then why add it?
In the 'default' branch is an assert(false); Call it defensive programming.
>>+ char buf[BUFSIZ];
>>+
>>+ if (family == NFPROTO_IPV4) {
>>+ if (!numeric && addr->ip == 0) {
>>+ printf("anywhere ");
>>+ return;
>>+ }
>>+ if (numeric)
>>+ strcpy(buf, xtables_ipaddr_to_numeric(&addr->in));
>>+ else
>>+ strcpy(buf, xtables_ipaddr_to_anyname(&addr->in));
>>+ strcat(buf, xtables_ipmask_to_numeric(&mask->in));
>>+ printf("%s ", buf);
>
> There is no need to use the strcpy/strcat hacks. Just directly printf it.
As the comment says: "Shamelessly copied from libxt_conntrack.c". ;)
Furthermore I think it is good that way, because
xtables_ipaddr_to_numeric writes to a local static buffer, and
xtables_ipaddr_to_numeric might get called by
xtables_ipmask_to_numeric.
>>--- /dev/null
>>+++ b/extensions/libxt_ipvs.man
>>@@ -0,0 +1,7 @@
>>+ipvs tests where the packet was modified by IPVS, i.e. is the
>>+skb_buff->ipvs_property set.
>>+.TP
>>+[\fB!\fP] \fB--ipvs
>>+Does the packet have to IPVS property?
>>+
>>+TODO: Write proper documentation.
>
> Yes.
Sir, yes, sir ;) I am working on that.
Thanks,
-Hannes
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Bugme-new] [Bug 13760] New: 2.6.30 kernel locks up with pppoe in back trace (regression)
From: Eric Dumazet @ 2009-07-28 12:30 UTC (permalink / raw)
To: Pavel Emelyanov
Cc: Igor M Podlesny, Andrew Morton, bugzilla-daemon, bugme-daemon,
netdev, Paul E. McKenney, David S. Miller
In-Reply-To: <4A6ECA3A.4050309@openvz.org>
Pavel Emelyanov a écrit :
> Eric Dumazet wrote:
>> Igor M Podlesny a écrit :
>>> [...]
>>>> Could have been a problem in net core, perhaps.
>>>>
>>>> Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.
>>>>
>>>> It would help if we could see that trace, please. A digital photo
>>>> would suit.
>>> Here it is:
>>>
>>> http://bugzilla.kernel.org/attachment.cgi?id=22516
>>>
>>> (It's 2.6.30.3)
>>>
>> Looking at this, I believe net_assign_generic() is not safe.
>>
>> Two cpus could try to expand/update the array at same time, one update could be lost.
>>
>> register_pernet_gen_device() has a mutex to guard against concurrent
>> calls, but net_assign_generic() has no locking at all.
>>
>> I doubt this is the reason of the crash, still worth to mention it...
>>
>> [PATCH] net: net_assign_generic() is not SMP safe
>>
>> Two cpus could try to expand/update the array at same time, one update
>> could be lost during the copy of old array.
>
> How can this happen? The array is updated only during ->init routines
> of the pernet_operations, which are called from under the net_mutex.
>
> Do I miss anything?
>
Oops, I missed the obvious "BUG_ON(!mutex_is_locked(&net_mutex));"
Sorry for the noise and untested patch as well :)
>> Re-using net_mutex is an easy way to fix this, it was used right
>> before to allocate the 'id'
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>>
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index b7292a2..9c31ad1 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -467,15 +467,17 @@ int net_assign_generic(struct net *net, int id, void *data)
>> BUG_ON(!mutex_is_locked(&net_mutex));
>> BUG_ON(id == 0);
>>
>> + mutex_lock(&net_mutex);
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Krishna Kumar2 @ 2009-07-28 12:24 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, Jarek Poplawski, netdev
In-Reply-To: <20090728114803.GA28001@gondor.apana.org.au>
Hi Herbert,
Herbert Xu <herbert@gondor.apana.org.au> wrote on 07/28/2009 05:18:03 PM:
>
> Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
>
> On Tue, Jul 28, 2009 at 05:13:45PM +0530, Krishna Kumar2 wrote:
> > Sure, I will test and give my findings here. I guess setting the
> > flag in qdisc_create_dflt is the right place - that will cover pfifo,
> > bfifo and pfifo_fast (instead of attach_one_default_qdisc)?
>
> No I think you want to put it in attach_one_default_qdisc.
>
> Despite its name, qdisc_create_dflt is used by non-default qdiscs
> too.
Right, thanks for pointing this out.
BTW, I am testing another patch that does "Do not enqueue skb for
default qdisc if there are no skbs already on the queue" (to avoid
enqueue followed by immediate dequeue of the same skb for most xmit
operations on default qdiscs). I am planning to use the same flag
for both. I hope there is nothing wrong with that idea (excuse me
if this has already been discussed on the list but I haven't been
following for almost a year).
Thanks,
- KK
^ permalink raw reply
* [PATCH V2] CAN: Add Flexcan CAN controller driver
From: Sascha Hauer @ 2009-07-28 12:06 UTC (permalink / raw)
To: Socketcan-core; +Cc: Linux Netdev List
Hi,
Here is the second version of the flexcan driver.
Sascha
>From a1ea20bc862778f2b64efc18d21d05e5debb9562 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 21 Jul 2009 10:47:19 +0200
Subject: [PATCH] CAN: Add Flexcan CAN driver
This core is found on some Freescale SoCs and also some Coldfire
SoCs. Support for Coldfire is missing though at the moment as
they have an older revision of the core which does not have RX FIFO
support.
V2: integrated comments from Wolfgang Grandegger
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/net/can/Kconfig | 6 +
drivers/net/can/Makefile | 1 +
drivers/net/can/flexcan.c | 692 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 699 insertions(+), 0 deletions(-)
create mode 100644 drivers/net/can/flexcan.c
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 33821a8..99c6da4 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -74,6 +74,12 @@ config CAN_KVASER_PCI
This driver is for the the PCIcanx and PCIcan cards (1, 2 or
4 channel) from Kvaser (http://www.kvaser.com).
+config CAN_FLEXCAN
+ tristate "Support for Freescale FLEXCAN based chips"
+ depends on CAN_DEV
+ ---help---
+ Say Y here if you want to support for Freescale FlexCAN.
+
config CAN_DEBUG_DEVICES
bool "CAN devices debugging messages"
depends on CAN
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 523a941..25f2032 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -8,5 +8,6 @@ obj-$(CONFIG_CAN_DEV) += can-dev.o
can-dev-y := dev.o
obj-$(CONFIG_CAN_SJA1000) += sja1000/
+obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o
ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
new file mode 100644
index 0000000..8a2e820
--- /dev/null
+++ b/drivers/net/can/flexcan.c
@@ -0,0 +1,692 @@
+/*
+ * flexcan.c - FLEXCAN CAN controller driver
+ *
+ * Copyright (c) 2005-2006 Varma Electronics Oy
+ * Copyright (c) 2009 Sascha Hauer, Pengutronix
+ *
+ * Based on code originally by Andrey Volkov <avolkov@varma-el.com>
+ *
+ * LICENCE:
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/can/netlink.h>
+#include <linux/can/error.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/if_ether.h>
+#include <linux/can/dev.h>
+#include <linux/if_arp.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/list.h>
+#include <linux/can.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#define DRIVER_NAME "flexcan"
+
+/* FLEXCAN module configuration register (CANMCR) bits */
+#define CANMCR_MDIS (1 << 31)
+#define CANMCR_FRZ (1 << 30)
+#define CANMCR_FEN (1 << 29)
+#define CANMCR_HALT (1 << 28)
+#define CANMCR_SOFTRST (1 << 25)
+#define CANMCR_FRZACK (1 << 24)
+#define CANMCR_SUPV (1 << 23)
+#define CANMCR_SRX_DIS (1 << 17)
+#define CANMCR_MAXMB(x) ((x) & 0x0f)
+#define CANMCR_IDAM_A (0 << 8)
+#define CANMCR_IDAM_B (1 << 8)
+#define CANMCR_IDAM_C (2 << 8)
+
+/* FLEXCAN control register (CANCTRL) bits */
+#define CANCTRL_PRESDIV(x) (((x) & 0xff) << 24)
+#define CANCTRL_RJW(x) (((x) & 0x03) << 22)
+#define CANCTRL_PSEG1(x) (((x) & 0x07) << 19)
+#define CANCTRL_PSEG2(x) (((x) & 0x07) << 16)
+#define CANCTRL_BOFFMSK (1 << 15)
+#define CANCTRL_ERRMSK (1 << 14)
+#define CANCTRL_CLKSRC (1 << 13)
+#define CANCTRL_LPB (1 << 12)
+#define CANCTRL_TWRN_MSK (1 << 11)
+#define CANCTRL_RWRN_MSK (1 << 10)
+#define CANCTRL_SAMP (1 << 7)
+#define CANCTRL_BOFFREC (1 << 6)
+#define CANCTRL_TSYNC (1 << 5)
+#define CANCTRL_LBUF (1 << 4)
+#define CANCTRL_LOM (1 << 3)
+#define CANCTRL_PROPSEG(x) ((x) & 0x07)
+
+/* FLEXCAN error counter register (ERRCNT) bits */
+#define ERRCNT_REXECTR(x) (((x) & 0xff) << 8)
+#define ERRCNT_TXECTR(x) ((x) & 0xff)
+
+/* FLEXCAN error and status register (ERRSTAT) bits */
+#define ERRSTAT_TWRNINT (1 << 17)
+#define ERRSTAT_RWRNINT (1 << 16)
+#define ERRSTAT_BIT1ERR (1 << 15)
+#define ERRSTAT_BIT0ERR (1 << 14)
+#define ERRSTAT_ACKERR (1 << 13)
+#define ERRSTAT_CRCERR (1 << 12)
+#define ERRSTAT_FRMERR (1 << 11)
+#define ERRSTAT_STFERR (1 << 10)
+#define ERRSTAT_TXWRN (1 << 9)
+#define ERRSTAT_RXWRN (1 << 8)
+#define ERRSTAT_IDLE (1 << 7)
+#define ERRSTAT_TXRX (1 << 6)
+#define ERRSTAT_FLTCONF_MASK (3 << 4)
+#define ERRSTAT_FLTCONF_ERROR_ACTIVE (0 << 4)
+#define ERRSTAT_FLTCONF_ERROR_PASSIVE (1 << 4)
+#define ERRSTAT_FLTCONF_ERROR_BUS_OFF (2 << 4)
+#define ERRSTAT_BOFFINT (1 << 2)
+#define ERRSTAT_ERRINT (1 << 1)
+#define ERRSTAT_WAKINT (1 << 0)
+#define ERRSTAT_INT (ERRSTAT_BOFFINT | ERRSTAT_ERRINT | ERRSTAT_TWRNINT | \
+ ERRSTAT_RWRNINT)
+
+/* FLEXCAN interrupt flag register (IFLAG) bits */
+#define IFLAG_BUF(x) (1 << (x))
+#define IFLAG_RX_FIFO_OVERFLOW (1 << 7)
+#define IFLAG_RX_FIFO_WARN (1 << 6)
+#define IFLAG_RX_FIFO_AVAILABLE (1 << 5)
+
+/* FLEXCAN message buffers */
+#define MB_CNT_CODE(x) (((x) & 0xf) << 24)
+#define MB_CNT_SRR (1 << 22)
+#define MB_CNT_IDE (1 << 21)
+#define MB_CNT_RTR (1 << 20)
+#define MB_CNT_LENGTH(x) (((x) & 0xf) << 16)
+#define MB_CNT_TIMESTAMP(x) ((x) & 0xffff)
+
+#define MB_ID_STD (0x7ff << 18)
+#define MB_ID_EXT 0x1fffffff
+#define MB_CODE_MASK 0xf0ffffff
+
+/* Structure of the message buffer */
+struct flexcan_mb {
+ u32 can_dlc;
+ u32 can_id;
+ u32 data[2];
+};
+
+/* Structure of the hardware registers */
+struct flexcan_regs {
+ u32 canmcr; /* 0x00 */
+ u32 canctrl; /* 0x04 */
+ u32 timer; /* 0x08 */
+ u32 reserved1; /* 0x0c */
+ u32 rxgmask; /* 0x10 */
+ u32 rx14mask; /* 0x14 */
+ u32 rx15mask; /* 0x18 */
+ u32 errcnt; /* 0x1c */
+ u32 errstat; /* 0x20 */
+ u32 imask2; /* 0x24 */
+ u32 imask1; /* 0x28 */
+ u32 iflag2; /* 0x2c */
+ u32 iflag1; /* 0x30 */
+ u32 reserved4[19];
+ struct flexcan_mb cantxfg[64];
+};
+
+struct flexcan_priv {
+ struct can_priv can;
+ void __iomem *base;
+
+ struct net_device *dev;
+ struct clk *clk;
+};
+
+static struct can_bittiming_const flexcan_bittiming_const = {
+ .name = DRIVER_NAME,
+ .tseg1_min = 1,
+ .tseg1_max = 16,
+ .tseg2_min = 2,
+ .tseg2_max = 8,
+ .sjw_max = 4,
+ .brp_min = 1,
+ .brp_max = 256,
+ .brp_inc = 1,
+};
+
+#define TX_BUF_ID 8
+
+static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+ struct can_frame *frame = (struct can_frame *)skb->data;
+ struct flexcan_priv *priv = netdev_priv(dev);
+ struct flexcan_regs __iomem *regs = priv->base;
+ u32 can_id;
+ u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
+ u32 *can_data;
+
+ netif_stop_queue(dev);
+
+ if (frame->can_id & CAN_EFF_FLAG) {
+ can_id = frame->can_id & MB_ID_EXT;
+ dlc |= MB_CNT_IDE | MB_CNT_SRR;
+ } else {
+ can_id = (frame->can_id & CAN_SFF_MASK) << 18;
+ }
+
+ if (frame->can_id & CAN_RTR_FLAG)
+ dlc |= MB_CNT_RTR;
+
+ writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
+ writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
+
+ can_data = (u32 *)frame->data;
+ writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]);
+ writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]);
+
+ writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
+
+ kfree_skb(skb);
+
+ return NETDEV_TX_OK;
+}
+
+static void flexcan_rx_frame(struct net_device *ndev,
+ struct flexcan_mb __iomem *mb)
+{
+ struct net_device_stats *stats = &ndev->stats;
+ struct sk_buff *skb;
+ struct can_frame *frame;
+ int ctrl = readl(&mb->can_dlc);
+ int length = (ctrl >> 16) & 0x0f;
+ u32 id;
+
+ skb = dev_alloc_skb(sizeof(struct can_frame));
+ if (!skb) {
+ stats->rx_dropped++;
+ return;
+ }
+
+ frame = (struct can_frame *)skb_put(skb,
+ sizeof(struct can_frame));
+
+ frame->can_dlc = length;
+ id = readl(&mb->can_id) & MB_ID_EXT;
+
+ if (ctrl & MB_CNT_IDE) {
+ frame->can_id = id & CAN_EFF_MASK;
+ frame->can_id |= CAN_EFF_FLAG;
+ if (ctrl & MB_CNT_RTR)
+ frame->can_id |= CAN_RTR_FLAG;
+ } else {
+ frame->can_id = id >> 18;
+ if (ctrl & MB_CNT_RTR)
+ frame->can_id |= CAN_RTR_FLAG;
+ }
+
+ *(u32 *)frame->data = be32_to_cpu(readl(&mb->data[0]));
+ *((u32 *)frame->data + 1) = be32_to_cpu(readl(&mb->data[1]));
+
+ stats->rx_packets++;
+ stats->rx_bytes += frame->can_dlc;
+ skb->dev = ndev;
+ skb->protocol = __constant_htons(ETH_P_CAN);
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+ netif_rx(skb);
+}
+
+static void flexcan_error(struct net_device *ndev, u32 stat)
+{
+ struct can_frame *cf;
+ struct sk_buff *skb;
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct net_device_stats *stats = &ndev->stats;
+ enum can_state state = priv->can.state;
+ int error_warning = 0, rx_errors = 0, tx_errors = 0;
+
+ skb = dev_alloc_skb(sizeof(struct can_frame));
+ if (!skb)
+ return;
+
+ skb->dev = ndev;
+ skb->protocol = __constant_htons(ETH_P_CAN);
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+ cf = (struct can_frame *)skb_put(skb, sizeof(*cf));
+ memset(cf, 0, sizeof(*cf));
+
+ cf->can_id = CAN_ERR_FLAG;
+ cf->can_dlc = CAN_ERR_DLC;
+
+ if (stat & ERRSTAT_RWRNINT) {
+ error_warning = 1;
+ cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
+ }
+
+ if (stat & ERRSTAT_TWRNINT) {
+ error_warning = 1;
+ cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
+ }
+
+ switch ((stat >> 4) & 0x3) {
+ case 0:
+ state = CAN_STATE_ERROR_ACTIVE;
+ break;
+ case 1:
+ state = CAN_STATE_ERROR_PASSIVE;
+ break;
+ default:
+ state = CAN_STATE_BUS_OFF;
+ break;
+ }
+
+ if (stat & ERRSTAT_BOFFINT) {
+ cf->can_id |= CAN_ERR_BUSOFF;
+ can_bus_off(ndev);
+ }
+
+ if (stat & ERRSTAT_BIT1ERR) {
+ rx_errors = 1;
+ cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+ cf->data[2] |= CAN_ERR_PROT_BIT1;
+ }
+
+ if (stat & ERRSTAT_BIT0ERR) {
+ rx_errors = 1;
+ cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+ cf->data[2] |= CAN_ERR_PROT_BIT0;
+ }
+
+ if (stat & ERRSTAT_FRMERR) {
+ rx_errors = 1;
+ cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+ cf->data[2] |= CAN_ERR_PROT_FORM;
+ }
+
+ if (stat & ERRSTAT_STFERR) {
+ rx_errors = 1;
+ cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+ cf->data[2] |= CAN_ERR_PROT_STUFF;
+ }
+
+
+ if (stat & ERRSTAT_ACKERR) {
+ tx_errors = 1;
+ cf->can_id |= CAN_ERR_ACK;
+ }
+
+ if (error_warning)
+ priv->can.can_stats.error_warning++;
+ if (rx_errors)
+ stats->rx_errors++;
+ if (tx_errors)
+ stats->tx_errors++;
+
+ priv->can.state = state;
+
+ netif_rx(skb);
+
+ ndev->last_rx = jiffies;
+ stats->rx_packets++;
+ stats->rx_bytes += cf->can_dlc;
+}
+
+static irqreturn_t flexcan_isr(int irq, void *dev_id)
+{
+ struct net_device *ndev = dev_id;
+ struct net_device_stats *stats = &ndev->stats;
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct flexcan_regs __iomem *regs = priv->base;
+ u32 iflags, errstat;
+
+ errstat = readl(®s->errstat);
+ if (errstat & ERRSTAT_INT) {
+ flexcan_error(ndev, errstat);
+ writel(errstat & ERRSTAT_INT, ®s->errstat);
+ }
+
+ iflags = readl(®s->iflag1);
+
+ if (iflags & IFLAG_RX_FIFO_OVERFLOW) {
+ writel(IFLAG_RX_FIFO_OVERFLOW, ®s->iflag1);
+ stats->rx_over_errors++;
+ stats->rx_errors++;
+ }
+
+ while (iflags & IFLAG_RX_FIFO_AVAILABLE) {
+ struct flexcan_mb __iomem *mb = ®s->cantxfg[0];
+
+ flexcan_rx_frame(ndev, mb);
+ writel(IFLAG_RX_FIFO_AVAILABLE, ®s->iflag1);
+ readl(®s->timer);
+ iflags = readl(®s->iflag1);
+ }
+
+ if (iflags & (1 << TX_BUF_ID)) {
+ stats->tx_packets++;
+ writel((1 << TX_BUF_ID), ®s->iflag1);
+ netif_wake_queue(ndev);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int flexcan_set_bittiming(struct net_device *ndev)
+{
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct can_bittiming *bt = &priv->can.bittiming;
+ struct flexcan_regs __iomem *regs = priv->base;
+ u32 reg;
+
+ reg = readl(®s->canctrl);
+ reg &= ~(CANCTRL_SAMP | CANCTRL_PRESDIV(0xff) |
+ CANCTRL_PSEG1(7) | CANCTRL_PSEG2(7));
+ reg |= CANCTRL_PRESDIV(bt->brp - 1) |
+ CANCTRL_PSEG1(bt->phase_seg1 - 1) |
+ CANCTRL_PSEG2(bt->phase_seg2 - 1) |
+ CANCTRL_RJW(3) |
+ CANCTRL_PROPSEG(bt->prop_seg - 1);
+ if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+ reg |= CANCTRL_SAMP;
+ writel(reg, ®s->canctrl);
+
+ dev_dbg(&ndev->dev, "setting canctrl=0x%08x\n", reg);
+
+ return 0;
+}
+
+static int flexcan_open(struct net_device *ndev)
+{
+ int ret, i;
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct flexcan_regs __iomem *regs = priv->base;
+ u32 reg;
+
+ ret = open_candev(ndev);
+ if (ret)
+ return ret;
+
+ ret = request_irq(ndev->irq, flexcan_isr, 0, DRIVER_NAME, ndev);
+ if (ret)
+ goto err_irq;
+
+ clk_enable(priv->clk);
+
+ reg = CANMCR_SOFTRST | CANMCR_HALT;
+ writel(CANMCR_SOFTRST, ®s->canmcr);
+ udelay(10);
+
+ if (readl(®s->canmcr) & CANMCR_SOFTRST) {
+ dev_err(&ndev->dev, "Failed to softreset can module.\n");
+ ret = -ENODEV;
+ goto err_reset;
+ }
+
+ /* Enable error and bus off interrupt */
+ reg = readl(®s->canctrl);
+ reg |= CANCTRL_CLKSRC | CANCTRL_ERRMSK | CANCTRL_BOFFMSK |
+ CANCTRL_BOFFREC | CANCTRL_TWRN_MSK | CANCTRL_TWRN_MSK;
+ writel(reg, ®s->canctrl);
+
+ /* Set lowest buffer transmitted first */
+ reg |= CANCTRL_LBUF;
+ writel(reg, ®s->canctrl);
+
+ for (i = 0; i < 64; i++) {
+ writel(0, ®s->cantxfg[i].can_dlc);
+ writel(0, ®s->cantxfg[i].can_id);
+ writel(0, ®s->cantxfg[i].data[0]);
+ writel(0, ®s->cantxfg[i].data[1]);
+
+ /* Put MB into rx queue */
+ writel(MB_CNT_CODE(0x04), ®s->cantxfg[i].can_dlc);
+ }
+
+ /* acceptance mask/acceptance code (accept everything) */
+ writel(0x0, ®s->rxgmask);
+ writel(0x0, ®s->rx14mask);
+ writel(0x0, ®s->rx15mask);
+
+ /* Enable flexcan module */
+ reg = readl(®s->canmcr);
+ reg &= ~(CANMCR_MDIS | CANMCR_FRZ);
+ reg |= CANMCR_IDAM_C | CANMCR_FEN;
+ writel(reg, ®s->canmcr);
+
+ /* Synchronize with the can bus */
+ reg &= ~CANMCR_HALT;
+ writel(reg, ®s->canmcr);
+
+ /* Enable interrupts */
+ writel(IFLAG_RX_FIFO_OVERFLOW | IFLAG_RX_FIFO_AVAILABLE |
+ IFLAG_BUF(TX_BUF_ID),
+ ®s->imask1);
+
+ netif_start_queue(ndev);
+
+ return 0;
+
+err_reset:
+ free_irq(ndev->irq, ndev);
+err_irq:
+ close_candev(ndev);
+
+ return ret;
+}
+
+static int flexcan_close(struct net_device *ndev)
+{
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct flexcan_regs __iomem *regs = priv->base;
+
+ netif_stop_queue(ndev);
+
+ /* Disable all interrupts */
+ writel(0, ®s->imask1);
+ free_irq(ndev->irq, ndev);
+
+ close_candev(ndev);
+
+ /* Disable module */
+ writel(CANMCR_MDIS, ®s->canmcr);
+ clk_disable(priv->clk);
+ return 0;
+}
+
+static int flexcan_set_mode(struct net_device *ndev, enum can_mode mode)
+{
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct flexcan_regs __iomem *regs = priv->base;
+ u32 reg;
+
+ switch (mode) {
+ case CAN_MODE_START:
+ reg = readl(®s->canctrl);
+ reg &= ~CANCTRL_BOFFREC;
+ writel(reg, ®s->canctrl);
+ reg |= CANCTRL_BOFFREC;
+ writel(reg, ®s->canctrl);
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+ if (netif_queue_stopped(ndev))
+ netif_wake_queue(ndev);
+ break;
+
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static const struct net_device_ops flexcan_netdev_ops = {
+ .ndo_open = flexcan_open,
+ .ndo_stop = flexcan_close,
+ .ndo_start_xmit = flexcan_start_xmit,
+};
+
+static int register_flexcandev(struct net_device *ndev)
+{
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct flexcan_regs __iomem *regs = priv->base;
+ u32 reg;
+
+ reg = readl(®s->canmcr);
+ reg &= ~CANMCR_MDIS;
+ writel(reg, ®s->canmcr);
+ udelay(100);
+ reg |= CANMCR_FRZ | CANMCR_HALT | CANMCR_FEN;
+ writel(reg, ®s->canmcr);
+
+ reg = readl(®s->canmcr);
+
+ /* Currently we only support newer versions of this core featuring
+ * a RX FIFO. Older cores found on some Coldfire derivates are not
+ * yet supported.
+ */
+ if (!(reg & CANMCR_FEN)) {
+ dev_err(&ndev->dev, "Could not enable RX FIFO, unsupported "
+ "core");
+ return -ENODEV;
+ }
+
+ ndev->flags |= IFF_ECHO; /* we support local echo in hardware */
+ ndev->netdev_ops = &flexcan_netdev_ops;
+
+ return register_candev(ndev);
+}
+
+static void unregister_flexcandev(struct net_device *ndev)
+{
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct flexcan_regs __iomem *regs = priv->base;
+ u32 reg;
+
+ reg = readl(®s->canmcr);
+ reg |= CANMCR_FRZ | CANMCR_HALT | CANMCR_MDIS;
+ writel(reg, ®s->canmcr);
+
+ unregister_candev(ndev);
+}
+
+static int __devinit flexcan_probe(struct platform_device *pdev)
+{
+ struct resource *mem;
+ struct net_device *ndev;
+ struct flexcan_priv *priv;
+ u32 mem_size;
+ int ret;
+
+ ndev = alloc_candev(sizeof(struct flexcan_priv));
+ if (!ndev)
+ return -ENOMEM;
+
+ priv = netdev_priv(ndev);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ ndev->irq = platform_get_irq(pdev, 0);
+ if (!mem || !ndev->irq) {
+ ret = -ENODEV;
+ goto failed_req;
+ }
+
+ mem_size = resource_size(mem);
+
+ if (!request_mem_region(mem->start, mem_size, DRIVER_NAME)) {
+ ret = -EBUSY;
+ goto failed_req;
+ }
+
+ SET_NETDEV_DEV(ndev, &pdev->dev);
+
+ priv->base = ioremap(mem->start, mem_size);
+ if (!priv->base) {
+ ret = -ENOMEM;
+ goto failed_map;
+ }
+
+ priv->clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ goto failed_clock;
+ }
+ priv->can.clock.freq = clk_get_rate(priv->clk);
+
+ platform_set_drvdata(pdev, ndev);
+
+ priv->can.do_set_bittiming = flexcan_set_bittiming;
+ priv->can.bittiming_const = &flexcan_bittiming_const;
+ priv->can.do_set_mode = flexcan_set_mode;
+
+ ret = register_flexcandev(ndev);
+ if (ret)
+ goto failed_register;
+
+ return 0;
+
+failed_register:
+ clk_put(priv->clk);
+failed_clock:
+ iounmap(priv->base);
+failed_map:
+ release_mem_region(mem->start, mem_size);
+failed_req:
+ free_candev(ndev);
+
+ return ret;
+}
+
+static int __devexit flexcan_remove(struct platform_device *pdev)
+{
+ struct net_device *ndev = platform_get_drvdata(pdev);
+ struct flexcan_priv *priv = netdev_priv(ndev);
+ struct resource *mem;
+
+ unregister_flexcandev(ndev);
+ platform_set_drvdata(pdev, NULL);
+ iounmap(priv->base);
+ clk_put(priv->clk);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(mem->start, resource_size(mem));
+ free_candev(ndev);
+
+ return 0;
+}
+
+static struct platform_driver flexcan_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+ .probe = flexcan_probe,
+ .remove = __devexit_p(flexcan_remove),
+};
+
+static int __init flexcan_init(void)
+{
+ return platform_driver_register(&flexcan_driver);
+}
+
+static void __exit flexcan_exit(void)
+{
+ platform_driver_unregister(&flexcan_driver);
+}
+
+module_init(flexcan_init);
+module_exit(flexcan_exit);
+
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CAN port driver for flexcan based chip");
--
1.6.3.3
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply related
* [PATCH 1/1] mISDN: Fix handling of receive buffer size in L1oIP
From: Karsten Keil @ 2009-07-27 17:24 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-kernel, netdev, David Miller, i4ldeveloper,
Andreas Eversberg, Arnaldo Carvalho de Melo
In-Reply-To: <cover.1248781021.git.keil@b1-systems.de>
From: Andreas Eversberg <andreas@eversberg.eu>
The size of receive buffer pointer was used to get size of
receive buffer instead of recvbuf_size itself, so only 4/8
bytes could be transfered.
This is a regression to 2.6.30 introduced by commit 8c90e11e3543d7de612194a042a148caeaab5f1d
mISDN: Use kernel_{send,recv}msg instead of open coding
Signed-off-by: Andreas Eversberg <andreas@eversberg.eu>
Signed-off-by: Karsten Keil <keil@b1-systems.de>
---
drivers/isdn/mISDN/l1oip_core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 990e6a7..c3b661a 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -731,10 +731,10 @@ l1oip_socket_thread(void *data)
while (!signal_pending(current)) {
struct kvec iov = {
.iov_base = recvbuf,
- .iov_len = sizeof(recvbuf),
+ .iov_len = recvbuf_size,
};
recvlen = kernel_recvmsg(socket, &msg, &iov, 1,
- sizeof(recvbuf), 0);
+ recvbuf_size, 0);
if (recvlen > 0) {
l1oip_socket_parse(hc, &sin_rx, recvbuf, recvlen);
} else {
--
1.6.0.2
^ permalink raw reply related
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Herbert Xu @ 2009-07-28 11:48 UTC (permalink / raw)
To: Krishna Kumar2; +Cc: Jarek Poplawski, David Miller, netdev
In-Reply-To: <OFD1BE2D07.6BDFA87B-ON65257601.003F8F24-65257601.00406E62@in.ibm.com>
On Tue, Jul 28, 2009 at 05:13:45PM +0530, Krishna Kumar2 wrote:
> Sure, I will test and give my findings here. I guess setting the
> flag in qdisc_create_dflt is the right place - that will cover pfifo,
> bfifo and pfifo_fast (instead of attach_one_default_qdisc)?
No I think you want to put it in attach_one_default_qdisc.
Despite its name, qdisc_create_dflt is used by non-default qdiscs
too.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Krishna Kumar2 @ 2009-07-28 11:43 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, Herbert Xu, netdev
In-Reply-To: <20090728112445.GB6593@ff.dom.local>
Sure, I will test and give my findings here. I guess setting the
flag in qdisc_create_dflt is the right place - that will cover pfifo,
bfifo and pfifo_fast (instead of attach_one_default_qdisc)?
Thanks for all the suggestions.
- KK
Jarek Poplawski <jarkao2@gmail.com> wrote on 07/28/2009 04:54:45 PM:
> On Tue, Jul 28, 2009 at 06:53:29PM +0800, Herbert Xu wrote:
> > On Tue, Jul 28, 2009 at 09:02:29AM +0000, Jarek Poplawski wrote:
> > >
> > > If it's only about the name I'm OK with: (qdisc->flags &
> > > TCQ_F_DEFAULT_WITHOUT_PREJUDICE_MAYBE_NON_DEFAULT_IN_THE_FUTURE)
> > > or any other, too ;-)
> >
> > I see. Yes a flag should work.
>
> So, I hope Krishna will try if these requested comments save any gains
> of the original patch...
>
> Thanks,
> Jarek P.
^ permalink raw reply
* [PATCH 0/1] mISDN: Fix handling of receive buffer size in L1oIP
From: Karsten Keil @ 2009-07-28 11:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-kernel, netdev, David Miller, i4ldeveloper,
Andreas Eversberg, Arnaldo Carvalho de Melo
This patch fix a regression introduced in 2.6.31 with the commit
8c90e11e3543d7de612194a042a148caeaab5f1d "mISDN: Use kernel_{send,recv}msg instead of open coding".
Linus: you can pull the fix from
git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6-net-next for_linus
David for net-next-2.6 you can get it from:
git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6-net-next for_david
Andreas Eversberg (1):
mISDN: Fix handling of receive buffer size in L1oIP
drivers/isdn/mISDN/l1oip_core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Jarek Poplawski @ 2009-07-28 11:24 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, krkumar2, netdev
In-Reply-To: <20090728105329.GA27453@gondor.apana.org.au>
On Tue, Jul 28, 2009 at 06:53:29PM +0800, Herbert Xu wrote:
> On Tue, Jul 28, 2009 at 09:02:29AM +0000, Jarek Poplawski wrote:
> >
> > If it's only about the name I'm OK with: (qdisc->flags &
> > TCQ_F_DEFAULT_WITHOUT_PREJUDICE_MAYBE_NON_DEFAULT_IN_THE_FUTURE)
> > or any other, too ;-)
>
> I see. Yes a flag should work.
So, I hope Krishna will try if these requested comments save any gains
of the original patch...
Thanks,
Jarek P.
^ permalink raw reply
* Re: [RFC][PATCH 1/5] IPVS: prefix EnterFunction and LeaveFunction msg with "IPVS:"
From: Hannes Eder @ 2009-07-28 11:15 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: lvs-devel, netdev, netfilter-devel, linux-kernel
In-Reply-To: <alpine.LSU.2.00.0907272013070.20984@fbirervta.pbzchgretzou.qr>
On Mon, Jul 27, 2009 at 20:14, Jan Engelhardt<jengelh@medozas.de> wrote:
>
> On Monday 2009-07-27 15:46, Hannes Eder wrote:
>>
>>Now all printk messages from IPVS are prefixed with "IPVS:".
>>
>>+#define EnterFunction(level) \
>>+ do { \
>>+ if (level <= ip_vs_get_debug_level()) \
>>+ printk(KERN_DEBUG "IPVS: Enter: %s, %s line %i\n", \
>>+ __func__, __FILE__, __LINE__); \
>>+ } while (0)
>>+#define LeaveFunction(level) \
>>+ do { \
>>+ if (level <= ip_vs_get_debug_level()) \
>>+ printk(KERN_DEBUG "IPVS: Leave: %s, %s line %i\n", \
>>+ __func__, __FILE__, __LINE__); \
>>+ } while (0)
>
> I think you should rather make use of pr_fmt:
>
> <before any #includes>
> #define pr_fmt(x) "IPVS: " x
>
> And then use pr_<level>("Elvis has left the building") in code. This
> will add IPVS: automatically to all pr_* calls, alleviating the need
> to manually type it into all printks.
I like this idea. I'll come up with an extra patch, it does not fit
into this series anyway.
> Of course, if you only want it for the two defines here, scrap
> my idea :)
>
Cheers,
-Hannes
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Herbert Xu @ 2009-07-28 10:53 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, krkumar2, netdev
In-Reply-To: <20090728090229.GA6593@ff.dom.local>
On Tue, Jul 28, 2009 at 09:02:29AM +0000, Jarek Poplawski wrote:
>
> If it's only about the name I'm OK with: (qdisc->flags &
> TCQ_F_DEFAULT_WITHOUT_PREJUDICE_MAYBE_NON_DEFAULT_IN_THE_FUTURE)
> or any other, too ;-)
I see. Yes a flag should work.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ 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