Re: [PATCH] team: set IFF_SLAVE on team ports

[Chas Williams]

On 07/10/15 02:41, Jiri Pirko wrote:

Thu, Jul 09, 2015 at 05:36:55PM CEST, jblu...@infradead.org wrote:

On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko  wrote:

Thu, Jul 09, 2015 at 11:58:34AM CEST, jblu...@infradead.org wrote:

The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
decide if it should start the address configuration. Since team ports
shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
a port to the team master.


I don't want to use IFF_SLAVE in team. Other master-slave devices are
not using that as well, for example bridge, ovs, etc.



Maybe they need to get fixed too. I've used that flag because it is
documented as
a "slave of a load balancer" which describes what a team port is.



I think that this should be fixed in addrconf_notify. It should lookup
if there is a master on top and bail out in that case.


There are other virtual interfaces that have a master assigned and want to
participate in IPv6 address configuration.


Can you give me an example?


I would like to revisit this patch (yes, I know it has been a while).  I 
believe the VRF implementation uses master to group the interfaces under

a single interface.

I don't see a reason not to use IFF_SLAVE since team and bonding are 
fairly similar.




Unless we want to have a cascade of conditionals testing the priv_flags in
addrconf_notify() this is asking for a new net_device_flags flag.
Maybe something
generic like IFF_L2PORT ?

Thanks,
Jan

[ Jiri, sorry for getting that mail twice ]

Re: [PATCH] team: set IFF_SLAVE on team ports

[Chas Williams]

On 07/10/15 02:41, Jiri Pirko wrote:

Thu, Jul 09, 2015 at 05:36:55PM CEST, jblu...@infradead.org wrote:

On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko  wrote:

Thu, Jul 09, 2015 at 11:58:34AM CEST, jblu...@infradead.org wrote:

The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
decide if it should start the address configuration. Since team ports
shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
a port to the team master.


I don't want to use IFF_SLAVE in team. Other master-slave devices are
not using that as well, for example bridge, ovs, etc.



Maybe they need to get fixed too. I've used that flag because it is
documented as
a "slave of a load balancer" which describes what a team port is.



I think that this should be fixed in addrconf_notify. It should lookup
if there is a master on top and bail out in that case.


There are other virtual interfaces that have a master assigned and want to
participate in IPv6 address configuration.


Can you give me an example?


I would like to revisit this patch (yes, I know it has been a while).  I 
believe the VRF implementation uses master to group the interfaces under

a single interface.

I don't see a reason not to use IFF_SLAVE since team and bonding are 
fairly similar.




Unless we want to have a cascade of conditionals testing the priv_flags in
addrconf_notify() this is asking for a new net_device_flags flag.
Maybe something
generic like IFF_L2PORT ?

Thanks,
Jan

[ Jiri, sorry for getting that mail twice ]

Re: net/atm: warning in alloc_tx/__might_sleep

[Chas Williams]

On Mon, 2017-03-13 at 18:43 +0100, Andrey Konovalov wrote:
> On Thu, Jan 12, 2017 at 11:40 AM, Chas Williams <3ch...@gmail.com> wrote:
> > On Wed, 2017-01-11 at 20:36 -0800, Cong Wang wrote:
> >> On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko <mho...@kernel.org> wrote:
> >> > On Wed 11-01-17 20:45:25, Michal Hocko wrote:
> >> >> On Wed 11-01-17 09:37:06, Chas Williams wrote:
> >> >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote:
> >> >> > > Hi!
> >> >> > >
> >> >> > > I've got the following error report while running the syzkaller 
> >> >> > > fuzzer.
> >> >> > >
> >> >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3).
> >> >> > >
> >> >> > > A reproducer is attached.
> >> >> > >
> >> >> > > [ cut here ]
> >> >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 
> >> >> > > __might_sleep+0x149/0x1a0
> >> >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at
> >> >> > > [] prepare_to_wait+0x182/0x530
> >> >> > > Modules linked in:
> >> >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59
> >> >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> >> >> > > 01/01/2011
> >> >> > > Call Trace:
> >> >> > >  __dump_stack lib/dump_stack.c:15
> >> >> > >  dump_stack+0x292/0x398 lib/dump_stack.c:51
> >> >> > >  __warn+0x19f/0x1e0 kernel/panic.c:547
> >> >> > >  warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562
> >> >> > >  __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732
> >> >> > >  slab_pre_alloc_hook mm/slab.h:408
> >> >> > >  slab_alloc_node mm/slub.c:2634
> >> >> > >  kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744
> >> >> > >  __alloc_skb+0x10f/0x800 net/core/skbuff.c:219
> >> >> > >  alloc_skb ./include/linux/skbuff.h:926
> >> >> > >  alloc_tx net/atm/common.c:75
> >> >> >
> >> >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx().  The simplest
> >> >> > fix for this would be simply to switch this GFP_ATOMIC.  See if this 
> >> >> > is
> >> >> > any better.
> >> >> >
> >> >> > diff --git a/net/atm/common.c b/net/atm/common.c
> >> >> > index a3ca922..d84220c 100644
> >> >> > --- a/net/atm/common.c
> >> >> > +++ b/net/atm/common.c
> >> >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc 
> >> >> > *vcc, unsigned int size)
> >> >> >  sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
> >> >> > return NULL;
> >> >> > }
> >> >> > -   while (!(skb = alloc_skb(size, GFP_KERNEL)))
> >> >> > +   while (!(skb = alloc_skb(size, GFP_ATOMIC)))
> >> >> > schedule();
> >> >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> >> >> > atomic_add(skb->truesize, >sk_wmem_alloc);
> >> >>
> >> >> Blee, this code is just horrendous. But the "fix" is obviously broken!
> >> >> schedule() is just a noop if you do not change the task state and what
> >> >> you are just asking for is a never failing non sleeping allocation - aka
> >> >> a busy loop in the kernel!
> >> >
> >> > And btw. this while loop should be really turned into GFP_KERNEL |
> >> > __GFP_NOFAIL with and explanation why this allocation cannot possibly
> >> > fail.
> >>
> >> I think a nested loop is quite unnecessary, probably due to the code itself
> >> is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is
> >> in the inner
> >> loop, both seem to wait for a successful GFP allocation. The inner one
> >> is even more unnecessary.
> >>
> >> Of course, I am not surprised MM may already have a mechanism to do
> >> the similar logic.
> >>
> >> There maybe some reason ATM needs such a logic, although other proto
> >> could handle skb allocation failure quite well in ->sendmsg().
> >
> >
> > I can't think of any particular reason that it needs this loop here.  I 
> > suspect
> > that the loop for alloc_tx() predates the wait logic in ->sendmsg() and 
> > that the
> > original looping was in alloc_tx() initially and was simply never removed.  
> > Changes
> > here would date back to before the git conversion.
> >
> 
> Hi,
> 
> I'm still seeing this on 4495c08e84729385774601b5146d51d9e5849f81 (4.11-rc2).
> 
> Thanks!

David Miller just accepted a patch for net-next that should resolve this issue.

Re: net/atm: warning in alloc_tx/__might_sleep

[Chas Williams]

On Mon, 2017-03-13 at 18:43 +0100, Andrey Konovalov wrote:
> On Thu, Jan 12, 2017 at 11:40 AM, Chas Williams <3ch...@gmail.com> wrote:
> > On Wed, 2017-01-11 at 20:36 -0800, Cong Wang wrote:
> >> On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko  wrote:
> >> > On Wed 11-01-17 20:45:25, Michal Hocko wrote:
> >> >> On Wed 11-01-17 09:37:06, Chas Williams wrote:
> >> >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote:
> >> >> > > Hi!
> >> >> > >
> >> >> > > I've got the following error report while running the syzkaller 
> >> >> > > fuzzer.
> >> >> > >
> >> >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3).
> >> >> > >
> >> >> > > A reproducer is attached.
> >> >> > >
> >> >> > > [ cut here ]
> >> >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 
> >> >> > > __might_sleep+0x149/0x1a0
> >> >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at
> >> >> > > [] prepare_to_wait+0x182/0x530
> >> >> > > Modules linked in:
> >> >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59
> >> >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> >> >> > > 01/01/2011
> >> >> > > Call Trace:
> >> >> > >  __dump_stack lib/dump_stack.c:15
> >> >> > >  dump_stack+0x292/0x398 lib/dump_stack.c:51
> >> >> > >  __warn+0x19f/0x1e0 kernel/panic.c:547
> >> >> > >  warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562
> >> >> > >  __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732
> >> >> > >  slab_pre_alloc_hook mm/slab.h:408
> >> >> > >  slab_alloc_node mm/slub.c:2634
> >> >> > >  kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744
> >> >> > >  __alloc_skb+0x10f/0x800 net/core/skbuff.c:219
> >> >> > >  alloc_skb ./include/linux/skbuff.h:926
> >> >> > >  alloc_tx net/atm/common.c:75
> >> >> >
> >> >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx().  The simplest
> >> >> > fix for this would be simply to switch this GFP_ATOMIC.  See if this 
> >> >> > is
> >> >> > any better.
> >> >> >
> >> >> > diff --git a/net/atm/common.c b/net/atm/common.c
> >> >> > index a3ca922..d84220c 100644
> >> >> > --- a/net/atm/common.c
> >> >> > +++ b/net/atm/common.c
> >> >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc 
> >> >> > *vcc, unsigned int size)
> >> >> >  sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
> >> >> > return NULL;
> >> >> > }
> >> >> > -   while (!(skb = alloc_skb(size, GFP_KERNEL)))
> >> >> > +   while (!(skb = alloc_skb(size, GFP_ATOMIC)))
> >> >> > schedule();
> >> >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> >> >> > atomic_add(skb->truesize, >sk_wmem_alloc);
> >> >>
> >> >> Blee, this code is just horrendous. But the "fix" is obviously broken!
> >> >> schedule() is just a noop if you do not change the task state and what
> >> >> you are just asking for is a never failing non sleeping allocation - aka
> >> >> a busy loop in the kernel!
> >> >
> >> > And btw. this while loop should be really turned into GFP_KERNEL |
> >> > __GFP_NOFAIL with and explanation why this allocation cannot possibly
> >> > fail.
> >>
> >> I think a nested loop is quite unnecessary, probably due to the code itself
> >> is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is
> >> in the inner
> >> loop, both seem to wait for a successful GFP allocation. The inner one
> >> is even more unnecessary.
> >>
> >> Of course, I am not surprised MM may already have a mechanism to do
> >> the similar logic.
> >>
> >> There maybe some reason ATM needs such a logic, although other proto
> >> could handle skb allocation failure quite well in ->sendmsg().
> >
> >
> > I can't think of any particular reason that it needs this loop here.  I 
> > suspect
> > that the loop for alloc_tx() predates the wait logic in ->sendmsg() and 
> > that the
> > original looping was in alloc_tx() initially and was simply never removed.  
> > Changes
> > here would date back to before the git conversion.
> >
> 
> Hi,
> 
> I'm still seeing this on 4495c08e84729385774601b5146d51d9e5849f81 (4.11-rc2).
> 
> Thanks!

David Miller just accepted a patch for net-next that should resolve this issue.

Re: net/atm: warning in alloc_tx/__might_sleep

[Chas Williams]

On Wed, 2017-01-11 at 20:36 -0800, Cong Wang wrote:
> On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko <mho...@kernel.org> wrote:
> > On Wed 11-01-17 20:45:25, Michal Hocko wrote:
> >> On Wed 11-01-17 09:37:06, Chas Williams wrote:
> >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote:
> >> > > Hi!
> >> > >
> >> > > I've got the following error report while running the syzkaller fuzzer.
> >> > >
> >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3).
> >> > >
> >> > > A reproducer is attached.
> >> > >
> >> > > [ cut here ]
> >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 
> >> > > __might_sleep+0x149/0x1a0
> >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at
> >> > > [] prepare_to_wait+0x182/0x530
> >> > > Modules linked in:
> >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59
> >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> >> > > 01/01/2011
> >> > > Call Trace:
> >> > >  __dump_stack lib/dump_stack.c:15
> >> > >  dump_stack+0x292/0x398 lib/dump_stack.c:51
> >> > >  __warn+0x19f/0x1e0 kernel/panic.c:547
> >> > >  warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562
> >> > >  __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732
> >> > >  slab_pre_alloc_hook mm/slab.h:408
> >> > >  slab_alloc_node mm/slub.c:2634
> >> > >  kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744
> >> > >  __alloc_skb+0x10f/0x800 net/core/skbuff.c:219
> >> > >  alloc_skb ./include/linux/skbuff.h:926
> >> > >  alloc_tx net/atm/common.c:75
> >> >
> >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx().  The simplest
> >> > fix for this would be simply to switch this GFP_ATOMIC.  See if this is
> >> > any better.
> >> >
> >> > diff --git a/net/atm/common.c b/net/atm/common.c
> >> > index a3ca922..d84220c 100644
> >> > --- a/net/atm/common.c
> >> > +++ b/net/atm/common.c
> >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, 
> >> > unsigned int size)
> >> >  sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
> >> > return NULL;
> >> > }
> >> > -   while (!(skb = alloc_skb(size, GFP_KERNEL)))
> >> > +   while (!(skb = alloc_skb(size, GFP_ATOMIC)))
> >> > schedule();
> >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> >> > atomic_add(skb->truesize, >sk_wmem_alloc);
> >>
> >> Blee, this code is just horrendous. But the "fix" is obviously broken!
> >> schedule() is just a noop if you do not change the task state and what
> >> you are just asking for is a never failing non sleeping allocation - aka
> >> a busy loop in the kernel!
> >
> > And btw. this while loop should be really turned into GFP_KERNEL |
> > __GFP_NOFAIL with and explanation why this allocation cannot possibly
> > fail.
> 
> I think a nested loop is quite unnecessary, probably due to the code itself
> is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is
> in the inner
> loop, both seem to wait for a successful GFP allocation. The inner one
> is even more unnecessary.
> 
> Of course, I am not surprised MM may already have a mechanism to do
> the similar logic.
> 
> There maybe some reason ATM needs such a logic, although other proto
> could handle skb allocation failure quite well in ->sendmsg().


I can't think of any particular reason that it needs this loop here.  I suspect
that the loop for alloc_tx() predates the wait logic in ->sendmsg() and that the
original looping was in alloc_tx() initially and was simply never removed.  
Changes
here would date back to before the git conversion.

Re: net/atm: warning in alloc_tx/__might_sleep

[Chas Williams]

On Wed, 2017-01-11 at 20:36 -0800, Cong Wang wrote:
> On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko  wrote:
> > On Wed 11-01-17 20:45:25, Michal Hocko wrote:
> >> On Wed 11-01-17 09:37:06, Chas Williams wrote:
> >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote:
> >> > > Hi!
> >> > >
> >> > > I've got the following error report while running the syzkaller fuzzer.
> >> > >
> >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3).
> >> > >
> >> > > A reproducer is attached.
> >> > >
> >> > > [ cut here ]
> >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 
> >> > > __might_sleep+0x149/0x1a0
> >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at
> >> > > [] prepare_to_wait+0x182/0x530
> >> > > Modules linked in:
> >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59
> >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
> >> > > 01/01/2011
> >> > > Call Trace:
> >> > >  __dump_stack lib/dump_stack.c:15
> >> > >  dump_stack+0x292/0x398 lib/dump_stack.c:51
> >> > >  __warn+0x19f/0x1e0 kernel/panic.c:547
> >> > >  warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562
> >> > >  __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732
> >> > >  slab_pre_alloc_hook mm/slab.h:408
> >> > >  slab_alloc_node mm/slub.c:2634
> >> > >  kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744
> >> > >  __alloc_skb+0x10f/0x800 net/core/skbuff.c:219
> >> > >  alloc_skb ./include/linux/skbuff.h:926
> >> > >  alloc_tx net/atm/common.c:75
> >> >
> >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx().  The simplest
> >> > fix for this would be simply to switch this GFP_ATOMIC.  See if this is
> >> > any better.
> >> >
> >> > diff --git a/net/atm/common.c b/net/atm/common.c
> >> > index a3ca922..d84220c 100644
> >> > --- a/net/atm/common.c
> >> > +++ b/net/atm/common.c
> >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, 
> >> > unsigned int size)
> >> >  sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
> >> > return NULL;
> >> > }
> >> > -   while (!(skb = alloc_skb(size, GFP_KERNEL)))
> >> > +   while (!(skb = alloc_skb(size, GFP_ATOMIC)))
> >> > schedule();
> >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> >> > atomic_add(skb->truesize, >sk_wmem_alloc);
> >>
> >> Blee, this code is just horrendous. But the "fix" is obviously broken!
> >> schedule() is just a noop if you do not change the task state and what
> >> you are just asking for is a never failing non sleeping allocation - aka
> >> a busy loop in the kernel!
> >
> > And btw. this while loop should be really turned into GFP_KERNEL |
> > __GFP_NOFAIL with and explanation why this allocation cannot possibly
> > fail.
> 
> I think a nested loop is quite unnecessary, probably due to the code itself
> is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is
> in the inner
> loop, both seem to wait for a successful GFP allocation. The inner one
> is even more unnecessary.
> 
> Of course, I am not surprised MM may already have a mechanism to do
> the similar logic.
> 
> There maybe some reason ATM needs such a logic, although other proto
> could handle skb allocation failure quite well in ->sendmsg().


I can't think of any particular reason that it needs this loop here.  I suspect
that the loop for alloc_tx() predates the wait logic in ->sendmsg() and that the
original looping was in alloc_tx() initially and was simply never removed.  
Changes
here would date back to before the git conversion.

Re: net/atm: warning in alloc_tx/__might_sleep

[Chas Williams]

On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote:
> Hi!
> 
> I've got the following error report while running the syzkaller fuzzer.
> 
> On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3).
> 
> A reproducer is attached.
> 
> [ cut here ]
> WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 
> __might_sleep+0x149/0x1a0
> do not call blocking ops when !TASK_RUNNING; state=1 set at
> [] prepare_to_wait+0x182/0x530
> Modules linked in:
> CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:15
>  dump_stack+0x292/0x398 lib/dump_stack.c:51
>  __warn+0x19f/0x1e0 kernel/panic.c:547
>  warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562
>  __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732
>  slab_pre_alloc_hook mm/slab.h:408
>  slab_alloc_node mm/slub.c:2634
>  kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744
>  __alloc_skb+0x10f/0x800 net/core/skbuff.c:219
>  alloc_skb ./include/linux/skbuff.h:926
>  alloc_tx net/atm/common.c:75

This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx().  The simplest
fix for this would be simply to switch this GFP_ATOMIC.  See if this is
any better.

diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..d84220c 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned 
int size)
 sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
return NULL;
}
-   while (!(skb = alloc_skb(size, GFP_KERNEL)))
+   while (!(skb = alloc_skb(size, GFP_ATOMIC)))
schedule();
pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
atomic_add(skb->truesize, >sk_wmem_alloc);

Re: net/atm: warning in alloc_tx/__might_sleep

[Chas Williams]

On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote:
> Hi!
> 
> I've got the following error report while running the syzkaller fuzzer.
> 
> On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3).
> 
> A reproducer is attached.
> 
> [ cut here ]
> WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 
> __might_sleep+0x149/0x1a0
> do not call blocking ops when !TASK_RUNNING; state=1 set at
> [] prepare_to_wait+0x182/0x530
> Modules linked in:
> CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:15
>  dump_stack+0x292/0x398 lib/dump_stack.c:51
>  __warn+0x19f/0x1e0 kernel/panic.c:547
>  warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562
>  __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732
>  slab_pre_alloc_hook mm/slab.h:408
>  slab_alloc_node mm/slub.c:2634
>  kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744
>  __alloc_skb+0x10f/0x800 net/core/skbuff.c:219
>  alloc_skb ./include/linux/skbuff.h:926
>  alloc_tx net/atm/common.c:75

This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx().  The simplest
fix for this would be simply to switch this GFP_ATOMIC.  See if this is
any better.

diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..d84220c 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned 
int size)
 sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
return NULL;
}
-   while (!(skb = alloc_skb(size, GFP_KERNEL)))
+   while (!(skb = alloc_skb(size, GFP_ATOMIC)))
schedule();
pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
atomic_add(skb->truesize, >sk_wmem_alloc);

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/10/2016 09:02 AM, Boris Ostrovsky wrote:

On 11/10/2016 06:13 AM, Thomas Gleixner wrote:

On Thu, 10 Nov 2016, M. Vefa Bicakci wrote:


I have found that your patch unfortunately does not improve the situation
for me. Here is an excerpt obtained from the dmesg of a kernel compiled
with this patch *as well as* Sebastian's patch:
[0.002561] CPU: Physical Processor ID: 0
[0.002566] CPU: Processor Core ID: 0
[0.002572] [Firmware Bug]: CPU0: APIC id mismatch. Firmware:  CPUID: 2

So apic->cpu_present_to_apicid() gives us a completely bogus APIC id which
translates to a bogus package id. And looking at the XEN code:

   xen_pv_apic.cpu_present_to_apicid = xen_cpu_present_to_apicid,

and xen_cpu_present_to_apicid does:

static int xen_cpu_present_to_apicid(int cpu)
{
if (cpu_present(cpu))
return xen_get_apic_id(xen_apic_read(APIC_ID));
else
return BAD_APICID;
}

So independent of which present CPU we query we get just some random
information, in the above case we get BAD_APICID from xen_apic_read() not
from the else path as this CPU _IS_ present.

What's so wrong with storing the fricking firmware supplied APICid as
everybody else does and report it back when queried?


By firmware you mean ACPI? It is most likely not available to PV guests.
How about returning cpu_data(cpu).initial_apicid?

And what was the original problem?


The original issue I found was that VMware was returning a different set
of APIC id's in the ACPI tables than what it advertised on the CPU's.

http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1266716.html



This damned attitude of we just hack the code into submission and let
everybody else deal with the outcoming is utterly annoying.

Thanks,

tglx

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/10/2016 09:02 AM, Boris Ostrovsky wrote:

On 11/10/2016 06:13 AM, Thomas Gleixner wrote:

On Thu, 10 Nov 2016, M. Vefa Bicakci wrote:


I have found that your patch unfortunately does not improve the situation
for me. Here is an excerpt obtained from the dmesg of a kernel compiled
with this patch *as well as* Sebastian's patch:
[0.002561] CPU: Physical Processor ID: 0
[0.002566] CPU: Processor Core ID: 0
[0.002572] [Firmware Bug]: CPU0: APIC id mismatch. Firmware:  CPUID: 2

So apic->cpu_present_to_apicid() gives us a completely bogus APIC id which
translates to a bogus package id. And looking at the XEN code:

   xen_pv_apic.cpu_present_to_apicid = xen_cpu_present_to_apicid,

and xen_cpu_present_to_apicid does:

static int xen_cpu_present_to_apicid(int cpu)
{
if (cpu_present(cpu))
return xen_get_apic_id(xen_apic_read(APIC_ID));
else
return BAD_APICID;
}

So independent of which present CPU we query we get just some random
information, in the above case we get BAD_APICID from xen_apic_read() not
from the else path as this CPU _IS_ present.

What's so wrong with storing the fricking firmware supplied APICid as
everybody else does and report it back when queried?


By firmware you mean ACPI? It is most likely not available to PV guests.
How about returning cpu_data(cpu).initial_apicid?

And what was the original problem?


The original issue I found was that VMware was returning a different set
of APIC id's in the ACPI tables than what it advertised on the CPU's.

http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1266716.html



This damned attitude of we just hack the code into submission and let
everybody else deal with the outcoming is utterly annoying.

Thanks,

tglx

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/09/2016 10:57 PM, M. Vefa Bicakci wrote:

[0.002000] mvb: CPU: Physical Processor ID: 0
[0.002000] mvb: CPU: Processor Core ID: 0
[0.002000] mvb: identify_cpu:1112: c: 880013b0a040, c->logical_proc_id: 
65535
[0.002000] mvb: __default_cpu_present_to_apicid:612: Returning 65535! 
mps_cpu: 1, nr_cpu_ids: 2, cpu_present(mps_cpu): 1
[0.002000] smpboot: mvb: topology_update_package_map:270: cpu: 1, pkg: 4095
[0.002000] smpboot: APIC() Converting physical 4095 to logical package 0
[0.002000] smpboot: mvb: topology_update_package_map:305: cpu: 1, 
cpu_data(cpu).logical_proc_id: 0


This seems strange.  0x is BAD_APICID.  Why didn't this fail here:

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))  
<<
continue;
if (!topology_update_package_map(apicid, cpu))
continue;

topology_update_package_map() should never have been called?

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/09/2016 10:57 PM, M. Vefa Bicakci wrote:

[0.002000] mvb: CPU: Physical Processor ID: 0
[0.002000] mvb: CPU: Processor Core ID: 0
[0.002000] mvb: identify_cpu:1112: c: 880013b0a040, c->logical_proc_id: 
65535
[0.002000] mvb: __default_cpu_present_to_apicid:612: Returning 65535! 
mps_cpu: 1, nr_cpu_ids: 2, cpu_present(mps_cpu): 1
[0.002000] smpboot: mvb: topology_update_package_map:270: cpu: 1, pkg: 4095
[0.002000] smpboot: APIC() Converting physical 4095 to logical package 0
[0.002000] smpboot: mvb: topology_update_package_map:305: cpu: 1, 
cpu_data(cpu).logical_proc_id: 0


This seems strange.  0x is BAD_APICID.  Why didn't this fail here:

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))  
<<
continue;
if (!topology_update_package_map(apicid, cpu))
continue;

topology_update_package_map() should never have been called?

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/09/2016 10:35 AM, Thomas Gleixner wrote:

Both ACPI and MP specifications require that the APIC id in the respective
tables must be the same as the APIC id in CPUID.

The kernel retrieves the physical package id from the APIC id during the
ACPI/MP table scan and builds the physical to logical package map.

There exist Virtualbox and Xen implementations which violate the spec. As a
result the physical to logical package map, which relies on the ACPI/MP
tables does not work on those systems, because the CPUID initialized
physical package id does not match the firmware id. This causes system
crashes and malfunction due to invalid package mappings.

The only way to cure this is to sanitize the physical package id after the
CPUID enumeration and yell when the APIC ids are different. If the physical
package IDs differ use the package information from the ACPI/MP tables so
the existing logical package map just works.

Reported-by: "Charles (Chas) Williams" <ciwil...@brocade.com>,
Reported-by: M. Vefa Bicakci <m@runbox.com>
Signed-off-by: Thomas Gleixner <t...@linutronix.de>



For 4 virtual sockets, 1 core per socket VM:

[0.235459]  node  #0, CPUs:  #1
[0.238579] Disabled fast string operations
[0.238620] mce: CPU supports 0 MCE banks
[0.238864] [Firmware Bug]: CPU1: APIC id mismatch. Firmware: 1 CPUID: 2
[0.238878] [Firmware Bug]: CPU1: Using firmware package id 1 instead of 2
[0.239502]  #2
[0.241298] Disabled fast string operations
[0.241356] mce: CPU supports 0 MCE banks
[0.241429] [Firmware Bug]: CPU2: APIC id mismatch. Firmware: 2 CPUID: 4
[0.241431] [Firmware Bug]: CPU2: Using firmware package id 2 instead of 4
[0.241631]  #3
[0.244075] Disabled fast string operations
[0.244112] mce: CPU supports 0 MCE banks
[0.244284] [Firmware Bug]: CPU3: APIC id mismatch. Firmware: 3 CPUID: 6
[0.244293] [Firmware Bug]: CPU3: Using firmware package id 3 instead of 6

For a 2 virtual sockets, 2 cores per socket, VMware seems to get its
APIC table correct as this fixup code isn't triggered.  The mapping looks like:

[0.028911] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.029068] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0
[0.029072] topology_update_package_map:  apicid 1  pkg 0  cpu 1
[0.029220] topology_update_package_map:  apicid 2  pkg 1  cpu 2
[0.029376] smpboot: APIC(2) Converting physical 1 to logical package 1, cpu 
2
[0.029381] topology_update_package_map:  apicid 3  pkg 1  cpu 3
[0.029525] smpboot: Max logical packages: 2

For a VM with 1 virtual socket and 4 cores, the behavior is again correct.

[0.016198] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.016271] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 
0 (88023fc0a040)
[0.016273] topology_update_package_map:  apicid 1  pkg 0  cpu 1
[0.016336] topology_update_package_map:  apicid 2  pkg 0  cpu 2
[0.016397] topology_update_package_map:  apicid 3  pkg 0  cpu 3

It looks like VMware might have some assumption about the minimum number
of cores on a virtual socket.  Regardless, the fix solves my problem!

Thanks!

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/09/2016 10:35 AM, Thomas Gleixner wrote:

Both ACPI and MP specifications require that the APIC id in the respective
tables must be the same as the APIC id in CPUID.

The kernel retrieves the physical package id from the APIC id during the
ACPI/MP table scan and builds the physical to logical package map.

There exist Virtualbox and Xen implementations which violate the spec. As a
result the physical to logical package map, which relies on the ACPI/MP
tables does not work on those systems, because the CPUID initialized
physical package id does not match the firmware id. This causes system
crashes and malfunction due to invalid package mappings.

The only way to cure this is to sanitize the physical package id after the
CPUID enumeration and yell when the APIC ids are different. If the physical
package IDs differ use the package information from the ACPI/MP tables so
the existing logical package map just works.

Reported-by: "Charles (Chas) Williams" ,
Reported-by: M. Vefa Bicakci 
Signed-off-by: Thomas Gleixner 



For 4 virtual sockets, 1 core per socket VM:

[0.235459]  node  #0, CPUs:  #1
[0.238579] Disabled fast string operations
[0.238620] mce: CPU supports 0 MCE banks
[0.238864] [Firmware Bug]: CPU1: APIC id mismatch. Firmware: 1 CPUID: 2
[0.238878] [Firmware Bug]: CPU1: Using firmware package id 1 instead of 2
[0.239502]  #2
[0.241298] Disabled fast string operations
[0.241356] mce: CPU supports 0 MCE banks
[0.241429] [Firmware Bug]: CPU2: APIC id mismatch. Firmware: 2 CPUID: 4
[0.241431] [Firmware Bug]: CPU2: Using firmware package id 2 instead of 4
[0.241631]  #3
[0.244075] Disabled fast string operations
[0.244112] mce: CPU supports 0 MCE banks
[0.244284] [Firmware Bug]: CPU3: APIC id mismatch. Firmware: 3 CPUID: 6
[0.244293] [Firmware Bug]: CPU3: Using firmware package id 3 instead of 6

For a 2 virtual sockets, 2 cores per socket, VMware seems to get its
APIC table correct as this fixup code isn't triggered.  The mapping looks like:

[0.028911] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.029068] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0
[0.029072] topology_update_package_map:  apicid 1  pkg 0  cpu 1
[0.029220] topology_update_package_map:  apicid 2  pkg 1  cpu 2
[0.029376] smpboot: APIC(2) Converting physical 1 to logical package 1, cpu 
2
[0.029381] topology_update_package_map:  apicid 3  pkg 1  cpu 3
[0.029525] smpboot: Max logical packages: 2

For a VM with 1 virtual socket and 4 cores, the behavior is again correct.

[0.016198] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.016271] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 
0 (88023fc0a040)
[0.016273] topology_update_package_map:  apicid 1  pkg 0  cpu 1
[0.016336] topology_update_package_map:  apicid 2  pkg 0  cpu 2
[0.016397] topology_update_package_map:  apicid 3  pkg 0  cpu 3

It looks like VMware might have some assumption about the minimum number
of cores on a virtual socket.  Regardless, the fix solves my problem!

Thanks!

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/09/2016 11:03 AM, Peter Zijlstra wrote:

On Wed, Nov 09, 2016 at 04:35:51PM +0100, Thomas Gleixner wrote:

Both ACPI and MP specifications require that the APIC id in the respective
tables must be the same as the APIC id in CPUID.

The kernel retrieves the physical package id from the APIC id during the
ACPI/MP table scan and builds the physical to logical package map.

There exist Virtualbox and Xen implementations which violate the spec. As a


ISTR it was VMware, not VirtualBox, but whatever.. they're both crazy
virt stuff.


Yes, this was VMware in particular.  It would be good to get this comment
right so as not to mislead anyone.


 /*
+ * The physical to logical package id mapping is initialized from the
+ * acpi/mptables information. Make sure that CPUID actually agrees with
+ * that.
+ */
+static void sanitize_package_id(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_SMP
+   unsigned int pkg, apicid, cpu = smp_processor_id();
+
+   apicid = apic->cpu_present_to_apicid(cpu);
+   pkg = apicid >> boot_cpu_data.x86_coreid_bits;
+
+   if (apicid != c->initial_apicid) {
+   pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID: 
%x\n",
+  cpu, apicid, c->initial_apicid);


Should we not also 'fix' c->initial_apicid ?


Since we have c->apicid and c->initial_apicid it seems reasonable to keep one
set to the "correct" value.  I don't think c->initial_apicid is used past
this.

I should have some tests on this patch later today.

Re: [PATCH] x86/cpuid: Deal with broken firmware once more

[Charles (Chas) Williams]

On 11/09/2016 11:03 AM, Peter Zijlstra wrote:

On Wed, Nov 09, 2016 at 04:35:51PM +0100, Thomas Gleixner wrote:

Both ACPI and MP specifications require that the APIC id in the respective
tables must be the same as the APIC id in CPUID.

The kernel retrieves the physical package id from the APIC id during the
ACPI/MP table scan and builds the physical to logical package map.

There exist Virtualbox and Xen implementations which violate the spec. As a


ISTR it was VMware, not VirtualBox, but whatever.. they're both crazy
virt stuff.


Yes, this was VMware in particular.  It would be good to get this comment
right so as not to mislead anyone.


 /*
+ * The physical to logical package id mapping is initialized from the
+ * acpi/mptables information. Make sure that CPUID actually agrees with
+ * that.
+ */
+static void sanitize_package_id(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_SMP
+   unsigned int pkg, apicid, cpu = smp_processor_id();
+
+   apicid = apic->cpu_present_to_apicid(cpu);
+   pkg = apicid >> boot_cpu_data.x86_coreid_bits;
+
+   if (apicid != c->initial_apicid) {
+   pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID: 
%x\n",
+  cpu, apicid, c->initial_apicid);


Should we not also 'fix' c->initial_apicid ?


Since we have c->apicid and c->initial_apicid it seems reasonable to keep one
set to the "correct" value.  I don't think c->initial_apicid is used past
this.

I should have some tests on this patch later today.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/08/2016 09:31 AM, Thomas Gleixner wrote:

On Tue, 8 Nov 2016, Charles (Chas) Williams wrote:

[0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.016398] smpboot: APIC(0) Converting physical 0 to logical
package 0, cpu 0 (88023fc0a040)
[0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
[0.016462] smpboot: APIC(1) Converting physical 1 to logical
package 1, cpu 1 (88023fd0a040)

So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
apicid but it certainly doesn't seem to the match the apicid in the
CPU's registers.  For whatever reason, my VMware system is reporting
that the second CPU has a local APIC ID of 2:


The initial information comes from MP tables or ACPI.


[0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0,
apicid 0, initial_apicid 0
...
[0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2,
apicid 2, initial_apicid 2


And the CPUID emulation tells something different. Sigh!


I was thinking it might be better to call topology_update_package_map()
at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
cpu could be setup during smp_init_package_map().


Perhaps, but that does not make the inconsistencies go away


By the time I know it's not consistent, there isn't anything I can do
about it.  I can't update the table to remove the bad information.

The other alternative, is to trust the ACPI and just update the
cpu_data's apicid in identify_cpu() to the value from the table.
The earlier kernels didn't seem to rely as much on this information.
But it does appear to be "wrong" in the APIC table.  From acpidump:

[02Ch 0044   1]Subtable Type : 00 [Processor Local APIC]
[02Dh 0045   1]   Length : 08
[02Eh 0046   1] Processor ID : 00
[02Fh 0047   1]Local Apic ID : 00
[030h 0048   4]Flags (decoded below) : 0001
   Processor Enabled : 1

[034h 0052   1]Subtable Type : 00 [Processor Local APIC]
[035h 0053   1]   Length : 08
[036h 0054   1] Processor ID : 01
[037h 0055   1]Local Apic ID : 01
[038h 0056   4]Flags (decoded below) : 0001
   Processor Enabled : 1

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/08/2016 09:31 AM, Thomas Gleixner wrote:

On Tue, 8 Nov 2016, Charles (Chas) Williams wrote:

[0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.016398] smpboot: APIC(0) Converting physical 0 to logical
package 0, cpu 0 (88023fc0a040)
[0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
[0.016462] smpboot: APIC(1) Converting physical 1 to logical
package 1, cpu 1 (88023fd0a040)

So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
apicid but it certainly doesn't seem to the match the apicid in the
CPU's registers.  For whatever reason, my VMware system is reporting
that the second CPU has a local APIC ID of 2:


The initial information comes from MP tables or ACPI.


[0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0,
apicid 0, initial_apicid 0
...
[0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2,
apicid 2, initial_apicid 2


And the CPUID emulation tells something different. Sigh!


I was thinking it might be better to call topology_update_package_map()
at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
cpu could be setup during smp_init_package_map().


Perhaps, but that does not make the inconsistencies go away


By the time I know it's not consistent, there isn't anything I can do
about it.  I can't update the table to remove the bad information.

The other alternative, is to trust the ACPI and just update the
cpu_data's apicid in identify_cpu() to the value from the table.
The earlier kernels didn't seem to rely as much on this information.
But it does appear to be "wrong" in the APIC table.  From acpidump:

[02Ch 0044   1]Subtable Type : 00 [Processor Local APIC]
[02Dh 0045   1]   Length : 08
[02Eh 0046   1] Processor ID : 00
[02Fh 0047   1]Local Apic ID : 00
[030h 0048   4]Flags (decoded below) : 0001
   Processor Enabled : 1

[034h 0052   1]Subtable Type : 00 [Processor Local APIC]
[035h 0053   1]   Length : 08
[036h 0054   1] Processor ID : 01
[037h 0055   1]Local Apic ID : 01
[038h 0056   4]Flags (decoded below) : 0001
   Processor Enabled : 1

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/07/2016 03:20 PM, Thomas Gleixner wrote:

On Mon, 7 Nov 2016, Charles (Chas) Williams wrote:

On 11/07/2016 11:19 AM, Thomas Gleixner wrote:

On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:

I don't know why the CPU's phys_proc_id is 2.


max_physical_pkg_id gets initialized via:

cpus = boot_cpu_data.x86_max_cores;
max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?


I have discovered that that is not the problem.  smp_init_package_map()
is calculating the physical core id using the following:

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

...
if (!topology_update_package_map(apicid, cpu))
continue;

...
int topology_update_package_map(unsigned int apicid, unsigned int cpu)
{
unsigned int new, pkg = apicid >>
boot_cpu_data.x86_coreid_bits;

But later when the secondary CPU's are identified they use a different
calculation using the local APIC ID from the CPU's registers:

static void generic_identify(struct cpuinfo_x86 *c)
...
if (c->cpuid_level >= 0x0001) {
c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
...
c->phys_proc_id = c->initial_apicid;

So at the end of identify_cpu() when the boot/hotplug assignment is
put back:

c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

topology_phys_to_logical_pkg() is returning an invalid logical processor
since one isn't configured.

It's not clear to me what the right thing to do is or which is right.


Nice detective work! So the issue is that the package mapping code honours
boot_cpu_data.x86_coreid_bit, while generic_identify does
not. boot_cpu_data.x86_coreid_bit is obviously 1 in your case. Tentative
fix below. I still need to gow through that maze and figure out what could
go wrong with that :(


I don't think that will fix the issue.  My deubugging leads me to believe
that boot_cpu_data.x86_coreid_bits is probably 0:

[0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.016398] smpboot: APIC(0) Converting physical 0 to logical 
package 0, cpu 0 (88023fc0a040)
[0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
[0.016462] smpboot: APIC(1) Converting physical 1 to logical 
package 1, cpu 1 (88023fd0a040)

So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
apicid but it certainly doesn't seem to the match the apicid in the
CPU's registers.  For whatever reason, my VMware system is reporting
that the second CPU has a local APIC ID of 2:

[0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0, apicid 
0, initial_apicid 0
...
[0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2, apicid 
2, initial_apicid 2

I was thinking it might be better to call topology_update_package_map()
at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
cpu could be setup during smp_init_package_map().

topology_update_package_map() is also poorly named it can create the
assignment, but can't update it (since it doesn't know the previous
mapping).



8<
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -905,6 +905,8 @@ static void detect_null_seg_behavior(str

 static void generic_identify(struct cpuinfo_x86 *c)
 {
+   unsigned int pkg;
+
c->extended_cpuid_level = 0;

if (!have_cpuid_p())
@@ -929,7 +931,8 @@ static void generic_identify(struct cpui
c->apicid = c->initial_apicid;
 # endif
 #endif
-   c->phys_proc_id = c->initial_apicid;
+   pkg = c->initial_apicid >> boot_cpu_data.x86_coreid_bits;
+   c->phys_proc_id = pkg;
}

get_model_name(c); /* Default name */

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/07/2016 03:20 PM, Thomas Gleixner wrote:

On Mon, 7 Nov 2016, Charles (Chas) Williams wrote:

On 11/07/2016 11:19 AM, Thomas Gleixner wrote:

On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:

I don't know why the CPU's phys_proc_id is 2.


max_physical_pkg_id gets initialized via:

cpus = boot_cpu_data.x86_max_cores;
max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?


I have discovered that that is not the problem.  smp_init_package_map()
is calculating the physical core id using the following:

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

...
if (!topology_update_package_map(apicid, cpu))
continue;

...
int topology_update_package_map(unsigned int apicid, unsigned int cpu)
{
unsigned int new, pkg = apicid >>
boot_cpu_data.x86_coreid_bits;

But later when the secondary CPU's are identified they use a different
calculation using the local APIC ID from the CPU's registers:

static void generic_identify(struct cpuinfo_x86 *c)
...
if (c->cpuid_level >= 0x0001) {
c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
...
c->phys_proc_id = c->initial_apicid;

So at the end of identify_cpu() when the boot/hotplug assignment is
put back:

c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

topology_phys_to_logical_pkg() is returning an invalid logical processor
since one isn't configured.

It's not clear to me what the right thing to do is or which is right.


Nice detective work! So the issue is that the package mapping code honours
boot_cpu_data.x86_coreid_bit, while generic_identify does
not. boot_cpu_data.x86_coreid_bit is obviously 1 in your case. Tentative
fix below. I still need to gow through that maze and figure out what could
go wrong with that :(


I don't think that will fix the issue.  My deubugging leads me to believe
that boot_cpu_data.x86_coreid_bits is probably 0:

[0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[0.016398] smpboot: APIC(0) Converting physical 0 to logical 
package 0, cpu 0 (88023fc0a040)
[0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
[0.016462] smpboot: APIC(1) Converting physical 1 to logical 
package 1, cpu 1 (88023fd0a040)

So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
apicid but it certainly doesn't seem to the match the apicid in the
CPU's registers.  For whatever reason, my VMware system is reporting
that the second CPU has a local APIC ID of 2:

[0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0, apicid 
0, initial_apicid 0
...
[0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2, apicid 
2, initial_apicid 2

I was thinking it might be better to call topology_update_package_map()
at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
cpu could be setup during smp_init_package_map().

topology_update_package_map() is also poorly named it can create the
assignment, but can't update it (since it doesn't know the previous
mapping).



8<
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -905,6 +905,8 @@ static void detect_null_seg_behavior(str

 static void generic_identify(struct cpuinfo_x86 *c)
 {
+   unsigned int pkg;
+
c->extended_cpuid_level = 0;

if (!have_cpuid_p())
@@ -929,7 +931,8 @@ static void generic_identify(struct cpui
c->apicid = c->initial_apicid;
 # endif
 #endif
-   c->phys_proc_id = c->initial_apicid;
+   pkg = c->initial_apicid >> boot_cpu_data.x86_coreid_bits;
+   c->phys_proc_id = pkg;
}

get_model_name(c); /* Default name */

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/07/2016 11:19 AM, Thomas Gleixner wrote:

On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:


On 11/02/2016 08:25 AM, Sebastian Andrzej Siewior wrote:

I am not sure if this a race with the new hotplug code or something that was
always there. Both (M. Vefa Bicakc and Charles) say that the box boots
sometimes fine (without the patch).  smp_store_boot_cpu_info() should have
run
before the notofoert and thus should have set the info properly. However I
got
the following bootlog from Charles with this patch:


I don't this this is a race.  Here is some debugging from the two CPU VM
(2 sockets, 1 core per socket).  In identify_cpu() we have:

/* The boot/hotplug time assigment got cleared, restore it */
c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

The values just after this:

[0.228306] identify_cpu: c 88023fd0a040  logical_proc_id 65535
c->phys_proc_id 2

So what's interesting here, is the phys_proc_id of 2 for CPU1:

int topology_phys_to_logical_pkg(unsigned int phys_pkg)
{
if (phys_pkg >= max_physical_pkg_id)
return -1;
return physical_to_logical_pkg[phys_pkg];
}

And we happen to know the max_physical_pkg_id is 2 in this case.
So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
assigned to the logical_proc_id.

I don't know why the CPU's phys_proc_id is 2.


max_physical_pkg_id gets initialized via:

cpus = boot_cpu_data.x86_max_cores;
max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?


I have discovered that that is not the problem.  smp_init_package_map()
is calculating the physical core id using the following:

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

...
if (!topology_update_package_map(apicid, cpu))
continue;

...
int topology_update_package_map(unsigned int apicid, unsigned int cpu)
{
unsigned int new, pkg = apicid >> boot_cpu_data.x86_coreid_bits;

But later when the secondary CPU's are identified they use a different
calculation using the local APIC ID from the CPU's registers:

static void generic_identify(struct cpuinfo_x86 *c)
...
if (c->cpuid_level >= 0x0001) {
c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
...
c->phys_proc_id = c->initial_apicid;

So at the end of identify_cpu() when the boot/hotplug assignment is
put back:

c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

topology_phys_to_logical_pkg() is returning an invalid logical processor
since one isn't configured.

It's not clear to me what the right thing to do is or which is right.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/07/2016 11:19 AM, Thomas Gleixner wrote:

On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:


On 11/02/2016 08:25 AM, Sebastian Andrzej Siewior wrote:

I am not sure if this a race with the new hotplug code or something that was
always there. Both (M. Vefa Bicakc and Charles) say that the box boots
sometimes fine (without the patch).  smp_store_boot_cpu_info() should have
run
before the notofoert and thus should have set the info properly. However I
got
the following bootlog from Charles with this patch:


I don't this this is a race.  Here is some debugging from the two CPU VM
(2 sockets, 1 core per socket).  In identify_cpu() we have:

/* The boot/hotplug time assigment got cleared, restore it */
c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

The values just after this:

[0.228306] identify_cpu: c 88023fd0a040  logical_proc_id 65535
c->phys_proc_id 2

So what's interesting here, is the phys_proc_id of 2 for CPU1:

int topology_phys_to_logical_pkg(unsigned int phys_pkg)
{
if (phys_pkg >= max_physical_pkg_id)
return -1;
return physical_to_logical_pkg[phys_pkg];
}

And we happen to know the max_physical_pkg_id is 2 in this case.
So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
assigned to the logical_proc_id.

I don't know why the CPU's phys_proc_id is 2.


max_physical_pkg_id gets initialized via:

cpus = boot_cpu_data.x86_max_cores;
max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?


I have discovered that that is not the problem.  smp_init_package_map()
is calculating the physical core id using the following:

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

...
if (!topology_update_package_map(apicid, cpu))
continue;

...
int topology_update_package_map(unsigned int apicid, unsigned int cpu)
{
unsigned int new, pkg = apicid >> boot_cpu_data.x86_coreid_bits;

But later when the secondary CPU's are identified they use a different
calculation using the local APIC ID from the CPU's registers:

static void generic_identify(struct cpuinfo_x86 *c)
...
if (c->cpuid_level >= 0x0001) {
c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
...
c->phys_proc_id = c->initial_apicid;

So at the end of identify_cpu() when the boot/hotplug assignment is
put back:

c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

topology_phys_to_logical_pkg() is returning an invalid logical processor
since one isn't configured.

It's not clear to me what the right thing to do is or which is right.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/04/2016 02:03 PM, Sebastian Andrzej Siewior wrote:

On 2016-11-04 08:20:37 [-0400], Charles (Chas) Williams wrote:

The initial CPU boots and is identified:

[0.009018] identify_boot_cpu
[0.009174] generic_identify: phys_proc_id is now 0
...
[0.009427] identify_cpu: before c 81ae2680  logical_proc_id 0  
c->phys_proc_id 0
[0.009506] identify_cpu: after c 81ae2680  logical_proc_id 65535  
c->phys_proc_id 0

So, this is fine because the APIC hasn't been scanned yet.  APIC
now gets scanned:

[0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 
0 (88023fc0a040)
[0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 
1 (88023fd0a040)
[0.015797] smpboot: Max logical packages: 2


where is the APICID here is comming from?


This comes from here:

unsigned int apicid = apic->cpu_present_to_apicid(cpu);

if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
continue;
if (!topology_update_package_map(apicid, cpu))

And I think this is the part that is "wrong".  The apicid appears to
be a logical CPU id.  I believe that in most cases this mapping comes
from x86_bios_cpu_apicid (or x86_cpu_to_apicid) which is generated in
generic_processor_info() which maps apicid's to logical cpu indexes.

Note that apic->cpu_present_to_apicid() is using just the cpu_index.

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
continue;
if (!topology_update_package_map(apicid, cpu))
continue;
pr_warn("CPU %u APICId %x disabled\n", cpu, apicid);
per_cpu(x86_bios_cpu_apicid, cpu) = BAD_APICID;
set_cpu_possible(cpu, false);
set_cpu_present(cpu, false);
}


So, at this point, I think everything is correct.  But now the secondary
CPU's "boot":

[0.236569] identify_secondary_cpu
[0.236620] generic_identify: phys_proc_id is now 2


so here is where fun starts. Xen has also
arch/x86/xen/smp.c::cpu_bringup() where the phys_proc_id is changed. But
isn't done for vmware but it might a place where they duct tape things.

How is this APIC id different from the earlier? I guess based on your
output that generic_identify() changes the content of phys_proc_id.


[0.236745] identify_cpu: before c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2
[0.236747] identify_cpu: after c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called
my second CPU, 2.  This is >= max_physical_pkg_id, so it is going to get
set to -1.


Now. max_physical_pkg_id is huge. The physical_to_logical_pkg array is
set to -1 on init so slot two has the value -1. That is what you see -
not the -1 because of ">= max_physical_pkg_id".


The comment at the end of identfy_cpu() says:

/* The boot/hotplug time assigment got cleared, restore it */

So, logical_proc_id being wrong here before restoration doesn't bother
me since I assume something in booting the secondary CPU's clears any
existing cpu data.

I know detect_extended_topology() is likely being called for both CPU's
and getting the right values (checking this now).  I don't know why
generic_identify() is resetting this value.


I don't know either. But it is clearly reading the apic id twice and
second approach is different from the first which leads to different
results. So if you figure out how the first APICID for the second CPU is
retrieved and then you see how it happens for the second time. There
must be a difference.


The phys core id from generic_identify() comes from the CPU's EBX register
so we _know_ this is right.

   if (c->cpuid_level >= 0x0001) {
c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
#ifdef CONFIG_X86_32
# ifdef CONFIG_SMP
c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
# else
c->apicid = c->initial_apicid;
# endif
#endif
c->phys_proc_id = c->initial_apicid;
}

The intel docs http://x86.renejeschke.de/html/file_module_x86_id_45.html
claims this is the Local APIC ID.  So it seems likely this is correct
value.  It's not clear it matter if this is the right value or not
though.  Even if this is the correct apicid, nothing knows about it.

An argument could be made that instead of checking the cpuid level, we
could just use the apicid based on the cpu index just like the other code.
It would be consistent at least then.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/04/2016 02:03 PM, Sebastian Andrzej Siewior wrote:

On 2016-11-04 08:20:37 [-0400], Charles (Chas) Williams wrote:

The initial CPU boots and is identified:

[0.009018] identify_boot_cpu
[0.009174] generic_identify: phys_proc_id is now 0
...
[0.009427] identify_cpu: before c 81ae2680  logical_proc_id 0  
c->phys_proc_id 0
[0.009506] identify_cpu: after c 81ae2680  logical_proc_id 65535  
c->phys_proc_id 0

So, this is fine because the APIC hasn't been scanned yet.  APIC
now gets scanned:

[0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 
0 (88023fc0a040)
[0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 
1 (88023fd0a040)
[0.015797] smpboot: Max logical packages: 2


where is the APICID here is comming from?


This comes from here:

unsigned int apicid = apic->cpu_present_to_apicid(cpu);

if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
continue;
if (!topology_update_package_map(apicid, cpu))

And I think this is the part that is "wrong".  The apicid appears to
be a logical CPU id.  I believe that in most cases this mapping comes
from x86_bios_cpu_apicid (or x86_cpu_to_apicid) which is generated in
generic_processor_info() which maps apicid's to logical cpu indexes.

Note that apic->cpu_present_to_apicid() is using just the cpu_index.

for_each_present_cpu(cpu) {
unsigned int apicid = apic->cpu_present_to_apicid(cpu);

if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
continue;
if (!topology_update_package_map(apicid, cpu))
continue;
pr_warn("CPU %u APICId %x disabled\n", cpu, apicid);
per_cpu(x86_bios_cpu_apicid, cpu) = BAD_APICID;
set_cpu_possible(cpu, false);
set_cpu_present(cpu, false);
}


So, at this point, I think everything is correct.  But now the secondary
CPU's "boot":

[0.236569] identify_secondary_cpu
[0.236620] generic_identify: phys_proc_id is now 2


so here is where fun starts. Xen has also
arch/x86/xen/smp.c::cpu_bringup() where the phys_proc_id is changed. But
isn't done for vmware but it might a place where they duct tape things.

How is this APIC id different from the earlier? I guess based on your
output that generic_identify() changes the content of phys_proc_id.


[0.236745] identify_cpu: before c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2
[0.236747] identify_cpu: after c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called
my second CPU, 2.  This is >= max_physical_pkg_id, so it is going to get
set to -1.


Now. max_physical_pkg_id is huge. The physical_to_logical_pkg array is
set to -1 on init so slot two has the value -1. That is what you see -
not the -1 because of ">= max_physical_pkg_id".


The comment at the end of identfy_cpu() says:

/* The boot/hotplug time assigment got cleared, restore it */

So, logical_proc_id being wrong here before restoration doesn't bother
me since I assume something in booting the secondary CPU's clears any
existing cpu data.

I know detect_extended_topology() is likely being called for both CPU's
and getting the right values (checking this now).  I don't know why
generic_identify() is resetting this value.


I don't know either. But it is clearly reading the apic id twice and
second approach is different from the first which leads to different
results. So if you figure out how the first APICID for the second CPU is
retrieved and then you see how it happens for the second time. There
must be a difference.


The phys core id from generic_identify() comes from the CPU's EBX register
so we _know_ this is right.

   if (c->cpuid_level >= 0x0001) {
c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
#ifdef CONFIG_X86_32
# ifdef CONFIG_SMP
c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
# else
c->apicid = c->initial_apicid;
# endif
#endif
c->phys_proc_id = c->initial_apicid;
}

The intel docs http://x86.renejeschke.de/html/file_module_x86_id_45.html
claims this is the Local APIC ID.  So it seems likely this is correct
value.  It's not clear it matter if this is the right value or not
though.  Even if this is the correct apicid, nothing knows about it.

An argument could be made that instead of checking the cpuid level, we
could just use the apicid based on the cpu index just like the other code.
It would be consistent at least then.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/03/2016 01:47 PM, Sebastian Andrzej Siewior wrote:

On 2016-11-02 18:47:49 [-0400], Charles (Chas) Williams wrote:

I don't this this is a race.  Here is some debugging from the two CPU VM
(2 sockets, 1 core per socket).  In identify_cpu() we have:

/* The boot/hotplug time assigment got cleared, restore it */
c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

The values just after this:

[0.228306] identify_cpu: c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So what's interesting here, is the phys_proc_id of 2 for CPU1:

int topology_phys_to_logical_pkg(unsigned int phys_pkg)
{
if (phys_pkg >= max_physical_pkg_id)
return -1;
return physical_to_logical_pkg[phys_pkg];
}

And we happen to know the max_physical_pkg_id is 2 in this case.
So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
assigned to the logical_proc_id.

I don't know why the CPU's phys_proc_id is 2.


This is the physical ID. You have two logical IDs (on your two sockets
machine). What is max_physical_pkg_id? In order to get that -1 you would
have to max_physical_pkg_id of 1 but code does
max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

and I would be a little surprised if this is 1.

Sebastian


The initial CPU boots and is identified:

[0.009018] identify_boot_cpu
[0.009174] generic_identify: phys_proc_id is now 0
...
[0.009427] identify_cpu: before c 81ae2680  logical_proc_id 0  
c->phys_proc_id 0
[0.009506] identify_cpu: after c 81ae2680  logical_proc_id 65535  
c->phys_proc_id 0

So, this is fine because the APIC hasn't been scanned yet.  APIC
now gets scanned:

[0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 
0 (88023fc0a040)
[0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 
1 (88023fd0a040)
[0.015797] smpboot: Max logical packages: 2

So, at this point, I think everything is correct.  But now the secondary
CPU's "boot":

[0.236569] identify_secondary_cpu
[0.236620] generic_identify: phys_proc_id is now 2

[0.236745] identify_cpu: before c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2
[0.236747] identify_cpu: after c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called
my second CPU, 2.  This is >= max_physical_pkg_id, so it is going to get
set to -1.

The comment at the end of identfy_cpu() says:

/* The boot/hotplug time assigment got cleared, restore it */

So, logical_proc_id being wrong here before restoration doesn't bother
me since I assume something in booting the secondary CPU's clears any
existing cpu data.

I know detect_extended_topology() is likely being called for both CPU's
and getting the right values (checking this now).  I don't know why
generic_identify() is resetting this value.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/03/2016 01:47 PM, Sebastian Andrzej Siewior wrote:

On 2016-11-02 18:47:49 [-0400], Charles (Chas) Williams wrote:

I don't this this is a race.  Here is some debugging from the two CPU VM
(2 sockets, 1 core per socket).  In identify_cpu() we have:

/* The boot/hotplug time assigment got cleared, restore it */
c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

The values just after this:

[0.228306] identify_cpu: c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So what's interesting here, is the phys_proc_id of 2 for CPU1:

int topology_phys_to_logical_pkg(unsigned int phys_pkg)
{
if (phys_pkg >= max_physical_pkg_id)
return -1;
return physical_to_logical_pkg[phys_pkg];
}

And we happen to know the max_physical_pkg_id is 2 in this case.
So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
assigned to the logical_proc_id.

I don't know why the CPU's phys_proc_id is 2.


This is the physical ID. You have two logical IDs (on your two sockets
machine). What is max_physical_pkg_id? In order to get that -1 you would
have to max_physical_pkg_id of 1 but code does
max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

and I would be a little surprised if this is 1.

Sebastian


The initial CPU boots and is identified:

[0.009018] identify_boot_cpu
[0.009174] generic_identify: phys_proc_id is now 0
...
[0.009427] identify_cpu: before c 81ae2680  logical_proc_id 0  
c->phys_proc_id 0
[0.009506] identify_cpu: after c 81ae2680  logical_proc_id 65535  
c->phys_proc_id 0

So, this is fine because the APIC hasn't been scanned yet.  APIC
now gets scanned:

[0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 
0 (88023fc0a040)
[0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 
1 (88023fd0a040)
[0.015797] smpboot: Max logical packages: 2

So, at this point, I think everything is correct.  But now the secondary
CPU's "boot":

[0.236569] identify_secondary_cpu
[0.236620] generic_identify: phys_proc_id is now 2

[0.236745] identify_cpu: before c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2
[0.236747] identify_cpu: after c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called
my second CPU, 2.  This is >= max_physical_pkg_id, so it is going to get
set to -1.

The comment at the end of identfy_cpu() says:

/* The boot/hotplug time assigment got cleared, restore it */

So, logical_proc_id being wrong here before restoration doesn't bother
me since I assume something in booting the secondary CPU's clears any
existing cpu data.

I know detect_extended_topology() is likely being called for both CPU's
and getting the right values (checking this now).  I don't know why
generic_identify() is resetting this value.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/02/2016 08:25 AM, Sebastian Andrzej Siewior wrote:

I am not sure if this a race with the new hotplug code or something that was
always there. Both (M. Vefa Bicakc and Charles) say that the box boots
sometimes fine (without the patch).  smp_store_boot_cpu_info() should have run
before the notofoert and thus should have set the info properly. However I got
the following bootlog from Charles with this patch:


I don't this this is a race.  Here is some debugging from the two CPU VM
(2 sockets, 1 core per socket).  In identify_cpu() we have:

/* The boot/hotplug time assigment got cleared, restore it */
c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

The values just after this:

[0.228306] identify_cpu: c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So what's interesting here, is the phys_proc_id of 2 for CPU1:

int topology_phys_to_logical_pkg(unsigned int phys_pkg)
{
if (phys_pkg >= max_physical_pkg_id)
return -1;
return physical_to_logical_pkg[phys_pkg];
}

And we happen to know the max_physical_pkg_id is 2 in this case.
So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
assigned to the logical_proc_id.

I don't know why the CPU's phys_proc_id is 2.

Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

[Charles (Chas) Williams]

On 11/02/2016 08:25 AM, Sebastian Andrzej Siewior wrote:

I am not sure if this a race with the new hotplug code or something that was
always there. Both (M. Vefa Bicakc and Charles) say that the box boots
sometimes fine (without the patch).  smp_store_boot_cpu_info() should have run
before the notofoert and thus should have set the info properly. However I got
the following bootlog from Charles with this patch:


I don't this this is a race.  Here is some debugging from the two CPU VM
(2 sockets, 1 core per socket).  In identify_cpu() we have:

/* The boot/hotplug time assigment got cleared, restore it */
c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

The values just after this:

[0.228306] identify_cpu: c 88023fd0a040  logical_proc_id 65535  
c->phys_proc_id 2

So what's interesting here, is the phys_proc_id of 2 for CPU1:

int topology_phys_to_logical_pkg(unsigned int phys_pkg)
{
if (phys_pkg >= max_physical_pkg_id)
return -1;
return physical_to_logical_pkg[phys_pkg];
}

And we happen to know the max_physical_pkg_id is 2 in this case.
So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
assigned to the logical_proc_id.

I don't know why the CPU's phys_proc_id is 2.

Re: [PREEMPT-RT] Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

On 10/28/2016 04:03 AM, Sebastian Andrzej Siewior wrote:

On 2016-10-27 15:00:32 [-0400], Charles (Chas) Williams wrote:

I assume "init_rapl_pmus: maxpkg 4" is from init_rapl_pmus() returning
topology_max_packages(). So it says 4 but then returns 65535 for CPU 2
and 3. That -1 comes probably from topology_update_package_map(). Could
you please send a complete boot log and try the following patch? This
one should fix your boot problem and disable RAPL if the info is
invalid.


But sometimes the topology info is correct and if I get lucky, the
package id could be valid for all the CPU's.  Given the behavior,
I have seen so far it makes me thing the RAPL isn't being emulated.
So even if I did boot onto a "valid" set of cores, would I always be
certain that I will be on those cores?


I don't what vmware does here. Nor do they ship source to check. So if
you have a big HW box with say two packages, it might make sense to give
this information to the guest _if_ the CPUs are pinned and the guest
never migrates.


Yes, I agree _if_.  That's why it simply isn't clear to me that we should
attempt do any RAPL at all for VMWare.  The current behavior doesn't seem
to make sense and I don't expect it to suddenly start acting reasonable.
Since I don't understand why some package id's are valid and others
are not, I would prefer not to trust any of the information as far as
enabling/disabling the RAPL monitoring.




Per your request in your next email:


One thing I forgot to ask: Could you please check if you get the same
pkgid reported for cpu 0-3 on a pre-v4.8 kernel? (before the hotplug
rework).


Our previous kernel was 4.4, and didn't use the logical package id:

I see.

Did the patch I sent fixed it for you and were you not able to test?


Yes, it does prevent RAPL from starting and loading.  From the boot log:

[2.711481] RAPL PMU: rapl pmu error: max package: 4 but CPU2 belongs to 
65535
[2.711639] rapl pmu error: max package: 4 but CPU2 belongs to 65535

This was consistent across several reboots.  I poked around in the
VM settings.  Apparently this guest is configured for four virtual
sockets with one core per socket.  Testing with two virtual sockets,
one core per socket:

[2.163177] RAPL PMU: rapl pmu error: max package: 2 but CPU1 belongs to 
65535
[2.163304] rapl pmu error: max package: 2 but CPU1 belongs to 65535

Booting with 1 virtual socket, 1 core per socket:

[1.750311] RAPL PMU: API unit is 2^-32 Joules, 3 fixed counters, 
10737418240 ms ovfl timer
[1.750312] RAPL PMU: hw unit of domain pp0-core 2^-0 Joules
[1.750313] RAPL PMU: hw unit of domain package 2^-0 Joules
[1.750314] RAPL PMU: hw unit of domain dram 2^-0 Joules

Booting with 1 virtual socket, 4 cores per socket:

[3.527298] RAPL PMU: API unit is 2^-32 Joules, 3 fixed counters, 
10737418240 ms ovfl timer
[3.527302] RAPL PMU: hw unit of domain pp0-core 2^-0 Joules
[3.527304] RAPL PMU: hw unit of domain package 2^-0 Joules
[3.527307] RAPL PMU: hw unit of domain dram 2^-0 Joules

So, it looks like VMWare tends to always get something wrong if you have
more than one virtual socket.  The above behavior was consistent across
several reboots.

Re: [PREEMPT-RT] Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

On 10/28/2016 04:03 AM, Sebastian Andrzej Siewior wrote:

On 2016-10-27 15:00:32 [-0400], Charles (Chas) Williams wrote:

I assume "init_rapl_pmus: maxpkg 4" is from init_rapl_pmus() returning
topology_max_packages(). So it says 4 but then returns 65535 for CPU 2
and 3. That -1 comes probably from topology_update_package_map(). Could
you please send a complete boot log and try the following patch? This
one should fix your boot problem and disable RAPL if the info is
invalid.


But sometimes the topology info is correct and if I get lucky, the
package id could be valid for all the CPU's.  Given the behavior,
I have seen so far it makes me thing the RAPL isn't being emulated.
So even if I did boot onto a "valid" set of cores, would I always be
certain that I will be on those cores?


I don't what vmware does here. Nor do they ship source to check. So if
you have a big HW box with say two packages, it might make sense to give
this information to the guest _if_ the CPUs are pinned and the guest
never migrates.


Yes, I agree _if_.  That's why it simply isn't clear to me that we should
attempt do any RAPL at all for VMWare.  The current behavior doesn't seem
to make sense and I don't expect it to suddenly start acting reasonable.
Since I don't understand why some package id's are valid and others
are not, I would prefer not to trust any of the information as far as
enabling/disabling the RAPL monitoring.




Per your request in your next email:


One thing I forgot to ask: Could you please check if you get the same
pkgid reported for cpu 0-3 on a pre-v4.8 kernel? (before the hotplug
rework).


Our previous kernel was 4.4, and didn't use the logical package id:

I see.

Did the patch I sent fixed it for you and were you not able to test?


Yes, it does prevent RAPL from starting and loading.  From the boot log:

[2.711481] RAPL PMU: rapl pmu error: max package: 4 but CPU2 belongs to 
65535
[2.711639] rapl pmu error: max package: 4 but CPU2 belongs to 65535

This was consistent across several reboots.  I poked around in the
VM settings.  Apparently this guest is configured for four virtual
sockets with one core per socket.  Testing with two virtual sockets,
one core per socket:

[2.163177] RAPL PMU: rapl pmu error: max package: 2 but CPU1 belongs to 
65535
[2.163304] rapl pmu error: max package: 2 but CPU1 belongs to 65535

Booting with 1 virtual socket, 1 core per socket:

[1.750311] RAPL PMU: API unit is 2^-32 Joules, 3 fixed counters, 
10737418240 ms ovfl timer
[1.750312] RAPL PMU: hw unit of domain pp0-core 2^-0 Joules
[1.750313] RAPL PMU: hw unit of domain package 2^-0 Joules
[1.750314] RAPL PMU: hw unit of domain dram 2^-0 Joules

Booting with 1 virtual socket, 4 cores per socket:

[3.527298] RAPL PMU: API unit is 2^-32 Joules, 3 fixed counters, 
10737418240 ms ovfl timer
[3.527302] RAPL PMU: hw unit of domain pp0-core 2^-0 Joules
[3.527304] RAPL PMU: hw unit of domain package 2^-0 Joules
[3.527307] RAPL PMU: hw unit of domain dram 2^-0 Joules

So, it looks like VMWare tends to always get something wrong if you have
more than one virtual socket.  The above behavior was consistent across
several reboots.

Re: [PREEMPT-RT] Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

On 10/25/2016 08:22 AM, Sebastian Andrzej Siewior wrote:

On 2016-10-21 17:03:56 [-0400], Charles (Chas) Williams wrote:

[3.107126] init_rapl_pmus: maxpkg 4

there! vmware bug. It probably worked by chance.


Yes, the behavior is a bit random.


I assume "init_rapl_pmus: maxpkg 4" is from init_rapl_pmus() returning
topology_max_packages(). So it says 4 but then returns 65535 for CPU 2
and 3. That -1 comes probably from topology_update_package_map(). Could
you please send a complete boot log and try the following patch? This
one should fix your boot problem and disable RAPL if the info is
invalid.


But sometimes the topology info is correct and if I get lucky, the
package id could be valid for all the CPU's.  Given the behavior,
I have seen so far it makes me thing the RAPL isn't being emulated.
So even if I did boot onto a "valid" set of cores, would I always be
certain that I will be on those cores?


diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 0a535cea8ff3..f5d85f2853d7 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -682,6 +682,15 @@ static int __init init_rapl_pmus(void)
 {
int maxpkg = topology_max_packages();
size_t size;
+   unsigned int cpu;
+
+   for_each_possible_cpu(cpu) {
+   if (topology_logical_package_id(cpu) >= maxpkg) {
+   pr_err("rapl pmu error: max package: %u but CPU%d belongs to 
%u\n",
+  maxpkg, cpu, topology_logical_package_id(cpu));
+   return -EINVAL;
+   }
+   }

size = sizeof(*rapl_pmus) + maxpkg * sizeof(struct rapl_pmu *);
rapl_pmus = kzalloc(size, GFP_KERNEL);


Per your request in your next email:


One thing I forgot to ask: Could you please check if you get the same
pkgid reported for cpu 0-3 on a pre-v4.8 kernel? (before the hotplug
rework).


Our previous kernel was 4.4, and didn't use the logical package id:

/* check if phys_is is already covered */
for_each_cpu(i, _cpu_mask) {
if (phys_id == topology_physical_package_id(i))
return;

Re: [PREEMPT-RT] Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

On 10/25/2016 08:22 AM, Sebastian Andrzej Siewior wrote:

On 2016-10-21 17:03:56 [-0400], Charles (Chas) Williams wrote:

[3.107126] init_rapl_pmus: maxpkg 4

there! vmware bug. It probably worked by chance.


Yes, the behavior is a bit random.


I assume "init_rapl_pmus: maxpkg 4" is from init_rapl_pmus() returning
topology_max_packages(). So it says 4 but then returns 65535 for CPU 2
and 3. That -1 comes probably from topology_update_package_map(). Could
you please send a complete boot log and try the following patch? This
one should fix your boot problem and disable RAPL if the info is
invalid.


But sometimes the topology info is correct and if I get lucky, the
package id could be valid for all the CPU's.  Given the behavior,
I have seen so far it makes me thing the RAPL isn't being emulated.
So even if I did boot onto a "valid" set of cores, would I always be
certain that I will be on those cores?


diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 0a535cea8ff3..f5d85f2853d7 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -682,6 +682,15 @@ static int __init init_rapl_pmus(void)
 {
int maxpkg = topology_max_packages();
size_t size;
+   unsigned int cpu;
+
+   for_each_possible_cpu(cpu) {
+   if (topology_logical_package_id(cpu) >= maxpkg) {
+   pr_err("rapl pmu error: max package: %u but CPU%d belongs to 
%u\n",
+  maxpkg, cpu, topology_logical_package_id(cpu));
+   return -EINVAL;
+   }
+   }

size = sizeof(*rapl_pmus) + maxpkg * sizeof(struct rapl_pmu *);
rapl_pmus = kzalloc(size, GFP_KERNEL);


Per your request in your next email:


One thing I forgot to ask: Could you please check if you get the same
pkgid reported for cpu 0-3 on a pre-v4.8 kernel? (before the hotplug
rework).


Our previous kernel was 4.4, and didn't use the logical package id:

/* check if phys_is is already covered */
for_each_cpu(i, _cpu_mask) {
if (phys_id == topology_physical_package_id(i))
return;

Re: [PREEMPT-RT] Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

On 10/21/2016 06:56 AM, Sebastian Andrzej Siewior wrote:

On 2016-10-20 16:27:55 [-0400], Charles (Chas) Williams wrote:

Recent 4.8 kernels have been oopsing when running under VMWare:


can you reproduce this on bare metal?


I can't get dedicated access to the specific bare metal since it is
running as a dedicated hypervisor.  I haven't seen this issue anywhere
else though with the 4.8 kernel.


[2.270203] BUG: unable to handle kernel NULL pointer dereference at 
0408
[2.270325] IP: [] rapl_cpu_online+0x59/0x70


can you check if pmu is NULL?


It's not.  The dereference at 0x408 and pmu->cpu being fairly early in
the struct seems to indicate that pmu wasn't pointing to 0 at the time
(but fairly close).  I should have noticed that earlier.


Is there a particular order guaranteed by the callbacks?  Will
rapl_cpu_prepare() always happen before online/offline?  Additionally,


yes, see include/linux/cpuhotplug.h. On CPU-up the array ids are invoked
from CPUHP_OFFLINE till CPUHP_ONLINE.


Yes, I see that now.  Thanks for the pointer!


If a callback (such as CPUHP_PERF_X86_RAPL_PREP) fail then we rollback
to the starting point (in case of CPU up it would be CPUHP_OFFLINE.


You'll like this, I just did a little printk debugging because it was
easier than trying to get a debugger running:

[3.107126] init_rapl_pmus: maxpkg 4
[3.107263] rapl_cpu_prepare: pmu 880234faa540  cpu 0  pkgid 0
[3.107400] rapl_cpu_prepare: pmu 880234faa600  cpu 1  pkgid 2
[3.107537] rapl_cpu_prepare: pmu 880234faa6c0  cpu 2  pkgid 
65535
[3.107662] rapl_cpu_online: pmu 880234faa540 cpu 0 pkgid 0
[3.107907] rapl_cpu_online: pmu 880234faa600 cpu 1 pkgid 2
[3.108133] rapl_cpu_online: pmu 880234faa6c0 cpu 2 pkgid 65535
[3.108333] rapl_cpu_online: pmu 880234faa6c0 cpu 3 pkgid 65535

where pkgid is topology_logical_package_id(cpu).

I can't understand why I don't see a cpu 3 during cpu prepare, when I
see one later.  The 65535 is a -1 from topology_phys_to_logical_pkg()
getting assigned to the logical_proc_id apparently.

So this is pretty puzzling.  Since this is a guest running under VMWare, I
don't know that there is any particular CPU pinning or emulation of RAPL.

It looks there was a proposal to not run in guests:

https://lkml.org/lkml/2015/12/3/559

Re: [PREEMPT-RT] Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

On 10/21/2016 06:56 AM, Sebastian Andrzej Siewior wrote:

On 2016-10-20 16:27:55 [-0400], Charles (Chas) Williams wrote:

Recent 4.8 kernels have been oopsing when running under VMWare:


can you reproduce this on bare metal?


I can't get dedicated access to the specific bare metal since it is
running as a dedicated hypervisor.  I haven't seen this issue anywhere
else though with the 4.8 kernel.


[2.270203] BUG: unable to handle kernel NULL pointer dereference at 
0408
[2.270325] IP: [] rapl_cpu_online+0x59/0x70


can you check if pmu is NULL?


It's not.  The dereference at 0x408 and pmu->cpu being fairly early in
the struct seems to indicate that pmu wasn't pointing to 0 at the time
(but fairly close).  I should have noticed that earlier.


Is there a particular order guaranteed by the callbacks?  Will
rapl_cpu_prepare() always happen before online/offline?  Additionally,


yes, see include/linux/cpuhotplug.h. On CPU-up the array ids are invoked
from CPUHP_OFFLINE till CPUHP_ONLINE.


Yes, I see that now.  Thanks for the pointer!


If a callback (such as CPUHP_PERF_X86_RAPL_PREP) fail then we rollback
to the starting point (in case of CPU up it would be CPUHP_OFFLINE.


You'll like this, I just did a little printk debugging because it was
easier than trying to get a debugger running:

[3.107126] init_rapl_pmus: maxpkg 4
[3.107263] rapl_cpu_prepare: pmu 880234faa540  cpu 0  pkgid 0
[3.107400] rapl_cpu_prepare: pmu 880234faa600  cpu 1  pkgid 2
[3.107537] rapl_cpu_prepare: pmu 880234faa6c0  cpu 2  pkgid 
65535
[3.107662] rapl_cpu_online: pmu 880234faa540 cpu 0 pkgid 0
[3.107907] rapl_cpu_online: pmu 880234faa600 cpu 1 pkgid 2
[3.108133] rapl_cpu_online: pmu 880234faa6c0 cpu 2 pkgid 65535
[3.108333] rapl_cpu_online: pmu 880234faa6c0 cpu 3 pkgid 65535

where pkgid is topology_logical_package_id(cpu).

I can't understand why I don't see a cpu 3 during cpu prepare, when I
see one later.  The 65535 is a -1 from topology_phys_to_logical_pkg()
getting assigned to the logical_proc_id apparently.

So this is pretty puzzling.  Since this is a guest running under VMWare, I
don't know that there is any particular CPU pinning or emulation of RAPL.

It looks there was a proposal to not run in guests:

https://lkml.org/lkml/2015/12/3/559

Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

Recent 4.8 kernels have been oopsing when running under VMWare:

[2.270203] BUG: unable to handle kernel NULL pointer dereference at 
0408
[2.270325] IP: [] rapl_cpu_online+0x59/0x70
[2.270448] PGD 0
[2.270570] Oops: 0002 [#1] SMP
[2.270693] Modules linked in:
[2.270815] CPU: 2 PID: 21 Comm: cpuhp/2 Not tainted 4.8.2-1-amd64-vyatta #1
[2.270938] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 04/14/2014
[2.271060] task: 8802361fc2c0 task.stack: 880236208000
[2.271183] RIP: 0010:[]  [] 
rapl_cpu_online+0x59/0x70
[2.271306] RSP: :88023620be68  EFLAGS: 00010246
[2.271428] RAX: 0004 RBX: 88023fd0d940 RCX: 
[2.271551] RDX: 0040 RSI: 0004 RDI: 0004
[2.271673] RBP: 0002 R08: fffc R09: 
[2.271796] R10:  R11:  R12: 0400
[2.271918] R13: 8802361fc2c0 R14: 8802361fc2c0 R15: 8802361fc2c0
[2.272041] FS:  () GS:88023fd0() 
knlGS:
[2.272163] CS:  0010 DS:  ES:  CR0: 80050033
[2.272286] CR2: 0408 CR3: 01a06000 CR4: 000406e0
[2.272408] Stack:
[2.272531]  88023fd0d940 0002 81a38240 
81061231
[2.272654]  8802361fc2c0 880237002180 8107ddcf 

[2.272776]  8802361a5a80 880237002180 8107dcb0 
81a6a380
[2.272899] Call Trace:
[2.273021]  [] ? cpuhp_thread_fun+0x31/0x100
[2.273144]  [] ? smpboot_thread_fn+0x11f/0x180
[2.273266]  [] ? sort_range+0x20/0x20
[2.273389]  [] ? kthread+0xca/0xe0
[2.273511]  [] ? ret_from_fork+0x1f/0x40
[2.273634]  [] ? kthread_park+0x50/0x50
[2.273757] Code: 00 00 48 83 c0 22 4c 8b 24 c1 48 c7 c0 30 a1 00 00 48 8b 
14 10 e8 a8 61 26 00 3b 05 b6 56 ae 00 7c 0e f0 48 0f a
[2.279445] RIP  [] rapl_cpu_online+0x59/0x70
[2.279568]  RSP 
[2.279690] CR2: 0408
[2.279813] ---[ end trace c95da920748eb432 ]---


gdb tells me:

(gdb) info line *(rapl_cpu_online+0x59)
Line 595 of "arch/x86/events/intel/rapl.c" starts at address 0x81012bb9 

   and ends at 0x81012bbe .

Which is:


target = cpumask_any_and(_cpu_mask, topology_core_cpumask(cpu));
if (target < nr_cpu_ids)
return 0;

cpumask_set_cpu(cpu, _cpu_mask);
pmu->cpu = cpu;  <<
return 0;

This code was recently changed by commit 8b5b773d6245138c
"perf/x86/intel/rapl: Convert to hotplug state machine" and it
appears that the setup is done as a callback:

/*
 * Install callbacks. Core will call them for each online cpu.
 */

ret = cpuhp_setup_state(CPUHP_PERF_X86_RAPL_PREP, "PERF_X86_RAPL_PREP",
rapl_cpu_prepare, NULL);
if (ret)
goto out;

ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_RAPL_ONLINE,
"AP_PERF_X86_RAPL_ONLINE",
rapl_cpu_online, rapl_cpu_offline);

Is there a particular order guaranteed by the callbacks?  Will
rapl_cpu_prepare() always happen before online/offline?  Additionally,
rapl_cpu_prepare() can fail to allocate pmu,

static int rapl_cpu_prepare(unsigned int cpu)
{
struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);

if (pmu)
return 0;

pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
if (!pmu)
return -ENOMEM;

But rapl_cpu_online() would have no idea about this.  What should be
done in this case?

Oops in rapl_cpu_prepare()

[Charles (Chas) Williams]

Recent 4.8 kernels have been oopsing when running under VMWare:

[2.270203] BUG: unable to handle kernel NULL pointer dereference at 
0408
[2.270325] IP: [] rapl_cpu_online+0x59/0x70
[2.270448] PGD 0
[2.270570] Oops: 0002 [#1] SMP
[2.270693] Modules linked in:
[2.270815] CPU: 2 PID: 21 Comm: cpuhp/2 Not tainted 4.8.2-1-amd64-vyatta #1
[2.270938] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 04/14/2014
[2.271060] task: 8802361fc2c0 task.stack: 880236208000
[2.271183] RIP: 0010:[]  [] 
rapl_cpu_online+0x59/0x70
[2.271306] RSP: :88023620be68  EFLAGS: 00010246
[2.271428] RAX: 0004 RBX: 88023fd0d940 RCX: 
[2.271551] RDX: 0040 RSI: 0004 RDI: 0004
[2.271673] RBP: 0002 R08: fffc R09: 
[2.271796] R10:  R11:  R12: 0400
[2.271918] R13: 8802361fc2c0 R14: 8802361fc2c0 R15: 8802361fc2c0
[2.272041] FS:  () GS:88023fd0() 
knlGS:
[2.272163] CS:  0010 DS:  ES:  CR0: 80050033
[2.272286] CR2: 0408 CR3: 01a06000 CR4: 000406e0
[2.272408] Stack:
[2.272531]  88023fd0d940 0002 81a38240 
81061231
[2.272654]  8802361fc2c0 880237002180 8107ddcf 

[2.272776]  8802361a5a80 880237002180 8107dcb0 
81a6a380
[2.272899] Call Trace:
[2.273021]  [] ? cpuhp_thread_fun+0x31/0x100
[2.273144]  [] ? smpboot_thread_fn+0x11f/0x180
[2.273266]  [] ? sort_range+0x20/0x20
[2.273389]  [] ? kthread+0xca/0xe0
[2.273511]  [] ? ret_from_fork+0x1f/0x40
[2.273634]  [] ? kthread_park+0x50/0x50
[2.273757] Code: 00 00 48 83 c0 22 4c 8b 24 c1 48 c7 c0 30 a1 00 00 48 8b 
14 10 e8 a8 61 26 00 3b 05 b6 56 ae 00 7c 0e f0 48 0f a
[2.279445] RIP  [] rapl_cpu_online+0x59/0x70
[2.279568]  RSP 
[2.279690] CR2: 0408
[2.279813] ---[ end trace c95da920748eb432 ]---


gdb tells me:

(gdb) info line *(rapl_cpu_online+0x59)
Line 595 of "arch/x86/events/intel/rapl.c" starts at address 0x81012bb9 

   and ends at 0x81012bbe .

Which is:


target = cpumask_any_and(_cpu_mask, topology_core_cpumask(cpu));
if (target < nr_cpu_ids)
return 0;

cpumask_set_cpu(cpu, _cpu_mask);
pmu->cpu = cpu;  <<
return 0;

This code was recently changed by commit 8b5b773d6245138c
"perf/x86/intel/rapl: Convert to hotplug state machine" and it
appears that the setup is done as a callback:

/*
 * Install callbacks. Core will call them for each online cpu.
 */

ret = cpuhp_setup_state(CPUHP_PERF_X86_RAPL_PREP, "PERF_X86_RAPL_PREP",
rapl_cpu_prepare, NULL);
if (ret)
goto out;

ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_RAPL_ONLINE,
"AP_PERF_X86_RAPL_ONLINE",
rapl_cpu_online, rapl_cpu_offline);

Is there a particular order guaranteed by the callbacks?  Will
rapl_cpu_prepare() always happen before online/offline?  Additionally,
rapl_cpu_prepare() can fail to allocate pmu,

static int rapl_cpu_prepare(unsigned int cpu)
{
struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);

if (pmu)
return 0;

pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
if (!pmu)
return -ENOMEM;

But rapl_cpu_online() would have no idea about this.  What should be
done in this case?

Re: [PATCH v3 1/1] atm: solos-pci: Replace simple_strtol by kstrtoint

[Charles (Chas) Williams]

On Thu, 2015-12-03 at 13:58 +0100, LABBE Corentin wrote:
> On Thu, Dec 03, 2015 at 06:26:31AM -0500, Charles (Chas) Williams wrote:
> > On Thu, 2015-12-03 at 09:06 +0100, LABBE Corentin wrote:
> > > @@ -357,11 +357,11 @@ static int process_status(struct solos_card *card, 
> > > int port, struct sk_buff *skb
> > >   if (!str)
> > >   return -EIO;
> > >  
> > > - ver = simple_strtol(str, NULL, 10);
> > > - if (ver < 1) {
> > > + err = kstrtoint(str, 10, );
> > > + if (err || ver < 1) {
> > >   dev_warn(>dev->dev, "Unexpected status interrupt version 
> > > %d\n",
> > >ver);
> > > - return -EIO;
> > > + return err;
> > 
> > 
> > If ver < 1 then you might return a 0 here.  Always returning -EIO is
> > probably just fine.
> > 
> 
> Hello
> 
> I think the best solution is to split the test, since returning error code 
> from kstrtoint was asked by David Miller.
> if (err)
>   return err;
> if (ver < 1)
>   return -EIO;
> Thanks
> Regards

That's fine.  You just shouldn't return 0 if the ver < 1.  This isn't
timing critical code.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/1] atm: solos-pci: Replace simple_strtol by kstrtoint

[Charles (Chas) Williams]

On Thu, 2015-12-03 at 09:06 +0100, LABBE Corentin wrote:
> @@ -357,11 +357,11 @@ static int process_status(struct solos_card *card, int 
> port, struct sk_buff *skb
>   if (!str)
>   return -EIO;
>  
> - ver = simple_strtol(str, NULL, 10);
> - if (ver < 1) {
> + err = kstrtoint(str, 10, );
> + if (err || ver < 1) {
>   dev_warn(>dev->dev, "Unexpected status interrupt version 
> %d\n",
>ver);
> - return -EIO;
> + return err;


If ver < 1 then you might return a 0 here.  Always returning -EIO is
probably just fine.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/1] atm: solos-pci: Replace simple_strtol by kstrtoint

[Charles (Chas) Williams]

On Thu, 2015-12-03 at 09:06 +0100, LABBE Corentin wrote:
> @@ -357,11 +357,11 @@ static int process_status(struct solos_card *card, int 
> port, struct sk_buff *skb
>   if (!str)
>   return -EIO;
>  
> - ver = simple_strtol(str, NULL, 10);
> - if (ver < 1) {
> + err = kstrtoint(str, 10, );
> + if (err || ver < 1) {
>   dev_warn(>dev->dev, "Unexpected status interrupt version 
> %d\n",
>ver);
> - return -EIO;
> + return err;


If ver < 1 then you might return a 0 here.  Always returning -EIO is
probably just fine.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/1] atm: solos-pci: Replace simple_strtol by kstrtoint

[Charles (Chas) Williams]

On Thu, 2015-12-03 at 13:58 +0100, LABBE Corentin wrote:
> On Thu, Dec 03, 2015 at 06:26:31AM -0500, Charles (Chas) Williams wrote:
> > On Thu, 2015-12-03 at 09:06 +0100, LABBE Corentin wrote:
> > > @@ -357,11 +357,11 @@ static int process_status(struct solos_card *card, 
> > > int port, struct sk_buff *skb
> > >   if (!str)
> > >   return -EIO;
> > >  
> > > - ver = simple_strtol(str, NULL, 10);
> > > - if (ver < 1) {
> > > + err = kstrtoint(str, 10, );
> > > + if (err || ver < 1) {
> > >   dev_warn(>dev->dev, "Unexpected status interrupt version 
> > > %d\n",
> > >ver);
> > > - return -EIO;
> > > + return err;
> > 
> > 
> > If ver < 1 then you might return a 0 here.  Always returning -EIO is
> > probably just fine.
> > 
> 
> Hello
> 
> I think the best solution is to split the test, since returning error code 
> from kstrtoint was asked by David Miller.
> if (err)
>   return err;
> if (ver < 1)
>   return -EIO;
> Thanks
> Regards

That's fine.  You just shouldn't return 0 if the ver < 1.  This isn't
timing critical code.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] atm: iphase: Fix misleading indention and return -ENOMEM on error

[Charles (Chas) Williams]

On Sat, 2015-10-10 at 21:47 +0200, Tillmann Heidsieck wrote:
> this series fixes two of them. The if(); warning would require
> restructuring the code to a larger extend. Beyond this there remains a
> whooping number of > 2k checkpatch.pl warnings and errors each. Those
> can be grouped into 
...
> Generally I would not mind cleaning all this up for those who have to
> make functional changes to the driver. However, I would like to know
> from the maintainers if such an afford would be welcome or not.

It doesn't bother me if you do this.  I can review it.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] atm: iphase: Fix misleading indention and return -ENOMEM on error

[Charles (Chas) Williams]

On Sat, 2015-10-10 at 21:47 +0200, Tillmann Heidsieck wrote:
> this series fixes two of them. The if(); warning would require
> restructuring the code to a larger extend. Beyond this there remains a
> whooping number of > 2k checkpatch.pl warnings and errors each. Those
> can be grouped into 
...
> Generally I would not mind cleaning all this up for those who have to
> make functional changes to the driver. However, I would like to know
> from the maintainers if such an afford would be welcome or not.

It doesn't bother me if you do this.  I can review it.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 07/12] atm: hide 'struct zatm_t_hist'

[Charles (Chas) Williams]

On Wed, 2015-09-30 at 13:26 +0200, Arnd Bergmann wrote:
> The zatm_t_hist structure is not used anywhere in the kernel, but is
> exported to user space. As we are trying to eliminate uses of time_t
> in the kernel for y2038 compatibility, the current definition triggers
> checking tools because it contains 'struct timeval'.
> 
> We can work around this by adding '#ifdef __KERNEL__'. I could not find
> out what the structure is actually used for, so this is the safe choice
> in case there is some user space tool that relies on the definition.
> 
> If we are sure that nothing in user space relies on the structure, we
> can instead remove the definition completely.

It was used by the ZATM_GETHIST ioctl which is long since gone in the
kernel driver.  You can just remove this.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 07/12] atm: hide 'struct zatm_t_hist'

[Charles (Chas) Williams]

On Wed, 2015-09-30 at 13:26 +0200, Arnd Bergmann wrote:
> The zatm_t_hist structure is not used anywhere in the kernel, but is
> exported to user space. As we are trying to eliminate uses of time_t
> in the kernel for y2038 compatibility, the current definition triggers
> checking tools because it contains 'struct timeval'.
> 
> We can work around this by adding '#ifdef __KERNEL__'. I could not find
> out what the structure is actually used for, so this is the safe choice
> in case there is some user space tool that relies on the definition.
> 
> If we are sure that nothing in user space relies on the structure, we
> can instead remove the definition completely.

It was used by the ZATM_GETHIST ioctl which is long since gone in the
kernel driver.  You can just remove this.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow

[Charles (Chas) Williams]

On Fri, 2015-05-15 at 15:02 +, David Laight wrote:
> From: Jason A. Donenfeld
> > Sent: 13 May 2015 19:34
> > Since elt->length is a u8, we can make this variable a u8. Then we can
> > do proper bounds checking more easily. Without this, a potentially
> > negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
> > resulting in a remotely exploitable heap overflow with network
> > supplied data.
> ...
> > diff --git a/drivers/staging/ozwpan/ozusbsvc1.c 
> > b/drivers/staging/ozwpan/ozusbsvc1.c
> > index d434d8c..cd6c63e 100644
> > --- a/drivers/staging/ozwpan/ozusbsvc1.c
> > +++ b/drivers/staging/ozwpan/ozusbsvc1.c
> > @@ -390,8 +390,10 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
> > case OZ_GET_DESC_RSP: {
> > struct oz_get_desc_rsp *body =
> > (struct oz_get_desc_rsp *)usb_hdr;
> > -   int data_len = elt->length -
> > +   u8 data_len = elt->length -
> > sizeof(struct oz_get_desc_rsp) + 1;
> > +   if (data_len > elt->length)
> > +   break;

This check already seems bogus?  It's really:

if (sizeof(struct oz_get_desc_rsp) - 1 < 0)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow

[Charles (Chas) Williams]

On Fri, 2015-05-15 at 15:02 +, David Laight wrote:
 From: Jason A. Donenfeld
  Sent: 13 May 2015 19:34
  Since elt-length is a u8, we can make this variable a u8. Then we can
  do proper bounds checking more easily. Without this, a potentially
  negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
  resulting in a remotely exploitable heap overflow with network
  supplied data.
 ...
  diff --git a/drivers/staging/ozwpan/ozusbsvc1.c 
  b/drivers/staging/ozwpan/ozusbsvc1.c
  index d434d8c..cd6c63e 100644
  --- a/drivers/staging/ozwpan/ozusbsvc1.c
  +++ b/drivers/staging/ozwpan/ozusbsvc1.c
  @@ -390,8 +390,10 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
  case OZ_GET_DESC_RSP: {
  struct oz_get_desc_rsp *body =
  (struct oz_get_desc_rsp *)usb_hdr;
  -   int data_len = elt-length -
  +   u8 data_len = elt-length -
  sizeof(struct oz_get_desc_rsp) + 1;
  +   if (data_len  elt-length)
  +   break;

This check already seems bogus?  It's really:

if (sizeof(struct oz_get_desc_rsp) - 1  0)


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1 net-next] net/atm/signaling.c: replace current->state by __set_current_state()

[chas williams - CONTRACTOR]

Instead of fixing code that never gets compiled/test, you could probably
just remove the #if WAIT_FOR_DAEMON code which would get rid of this
raw assignment.

See the #undef around line 268.

On Mon, 23 Feb 2015 18:31:07 +0100
Fabian Frederick  wrote:

> Use helper functions to access current->state.
> Direct assignments are prone to races and therefore buggy.
> 
> Thanks to Peter Zijlstra for the exact definition of the problem.
> 
> Suggested-By: Peter Zijlstra 
> Signed-off-by: Fabian Frederick 
> ---
>  net/atm/signaling.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/atm/signaling.c b/net/atm/signaling.c
> index 523bce7..0140832 100644
> --- a/net/atm/signaling.c
> +++ b/net/atm/signaling.c
> @@ -40,7 +40,7 @@ static void sigd_put_skb(struct sk_buff *skb)
>   pr_debug("atmsvc: waiting for signaling daemon...\n");
>   schedule();
>   }
> - current->state = TASK_RUNNING;
> + __set_current_state(TASK_RUNNING);
>   remove_wait_queue(_sleep, );
>  #else
>   if (!sigd) {

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1 net-next] net/atm/signaling.c: replace current-state by __set_current_state()

[chas williams - CONTRACTOR]

Instead of fixing code that never gets compiled/test, you could probably
just remove the #if WAIT_FOR_DAEMON code which would get rid of this
raw assignment.

See the #undef around line 268.

On Mon, 23 Feb 2015 18:31:07 +0100
Fabian Frederick f...@skynet.be wrote:

 Use helper functions to access current-state.
 Direct assignments are prone to races and therefore buggy.
 
 Thanks to Peter Zijlstra for the exact definition of the problem.
 
 Suggested-By: Peter Zijlstra pet...@infradead.org
 Signed-off-by: Fabian Frederick f...@skynet.be
 ---
  net/atm/signaling.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/net/atm/signaling.c b/net/atm/signaling.c
 index 523bce7..0140832 100644
 --- a/net/atm/signaling.c
 +++ b/net/atm/signaling.c
 @@ -40,7 +40,7 @@ static void sigd_put_skb(struct sk_buff *skb)
   pr_debug(atmsvc: waiting for signaling daemon...\n);
   schedule();
   }
 - current-state = TASK_RUNNING;
 + __set_current_state(TASK_RUNNING);
   remove_wait_queue(sigd_sleep, wait);
  #else
   if (!sigd) {

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [net-next PATCH v3 1/1] atm: remove deprecated use of pci api

[chas williams - CONTRACTOR]

On Fri, 16 Jan 2015 15:10:25 +0100
Quentin Lambert  wrote:


> > -u32 dma_addr = pci_map_single((struct pci_dev*)fore200e->bus_dev, 
> > virt_addr, size, direction);
> > +u32 dma_addr = dma_map_single(&((struct pci_dev *) 
> > fore200e->bus_dev)->dev, virt_addr, size, direction);
> >   
> >   DPRINTK(3, "PCI DVMA mapping: virt_addr = 0x%p, size = %d, direction 
> > = %d,  --> dma_addr = 0x%08x\n",
> > virt_addr, size, direction, dma_addr);
> >
> []
> 
> I am going try to make similar changes in some other part of the kernel and
> I was wondering if you could explain how you decided it wasn't necessary to
> check for "((struct pci_dev *) fore200e->bus_dev" nullity for instance.

This gets set up in fore200e_pca_detect() which is pretty early in the
intialization process.  We don't get as far as using any of the "DVMA"
stubs unless pci_enable_device() succeeds, meaning pci_dev is good, and
fore200e->bus_dev is assigned to pci_dev (around line 2724).

fore200e->bus_dev is never cleared back to NULL, but obviously you
shouldn't be using any of the DMA routines after disabling the pci
device.  Hopefully the driver shuts down in an orderly fashion such
that all DMA is over by the time the driver disables the pci device.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[net-next PATCH v3 1/1] atm: remove deprecated use of pci api

[chas williams - CONTRACTOR]

Signed-off-by: Chas Williams - CONTRACTOR 
---
 drivers/atm/eni.c   |  33 +++--
 drivers/atm/fore200e.c  |  22 +
 drivers/atm/he.c| 125 +---
 drivers/atm/he.h|   4 +-
 drivers/atm/idt77252.c  | 107 ++---
 drivers/atm/iphase.c|  54 +++--
 drivers/atm/lanai.c |  14 ++
 drivers/atm/nicstar.c   |  60 +++
 drivers/atm/solos-pci.c |  26 +-
 drivers/atm/zatm.c  |  17 ---
 10 files changed, 243 insertions(+), 219 deletions(-)

diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index c7fab3e..6339efd 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -354,9 +354,9 @@ static int do_rx_dma(struct atm_vcc *vcc,struct sk_buff 
*skb,
eni_vcc = ENI_VCC(vcc);
paddr = 0; /* GCC, shut up */
if (skb) {
-   paddr = pci_map_single(eni_dev->pci_dev,skb->data,skb->len,
-   PCI_DMA_FROMDEVICE);
-   if (pci_dma_mapping_error(eni_dev->pci_dev, paddr))
+   paddr = 
dma_map_single(_dev->pci_dev->dev,skb->data,skb->len,
+  DMA_FROM_DEVICE);
+   if (dma_mapping_error(_dev->pci_dev->dev, paddr))
goto dma_map_error;
ENI_PRV_PADDR(skb) = paddr;
if (paddr & 3)
@@ -481,8 +481,8 @@ rx_enqueued++;
 
 trouble:
if (paddr)
-   pci_unmap_single(eni_dev->pci_dev,paddr,skb->len,
-   PCI_DMA_FROMDEVICE);
+   dma_unmap_single(_dev->pci_dev->dev,paddr,skb->len,
+DMA_FROM_DEVICE);
 dma_map_error:
if (skb) dev_kfree_skb_irq(skb);
return -1;
@@ -758,8 +758,8 @@ rx_dequeued++;
}
eni_vcc->rxing--;
eni_vcc->rx_pos = ENI_PRV_POS(skb) & (eni_vcc->words-1);
-   pci_unmap_single(eni_dev->pci_dev,ENI_PRV_PADDR(skb),skb->len,
-   PCI_DMA_TODEVICE);
+   
dma_unmap_single(_dev->pci_dev->dev,ENI_PRV_PADDR(skb),skb->len,
+DMA_TO_DEVICE);
if (!skb->len) dev_kfree_skb_irq(skb);
else {
EVENT("pushing (len=%ld)\n",skb->len,0);
@@ -1112,8 +1112,8 @@ DPRINTK("iovcnt = %d\n",skb_shinfo(skb)->nr_frags);
vcc->dev->number);
return enq_jam;
}
-   paddr = pci_map_single(eni_dev->pci_dev,skb->data,skb->len,
-   PCI_DMA_TODEVICE);
+   paddr = dma_map_single(_dev->pci_dev->dev,skb->data,skb->len,
+  DMA_TO_DEVICE);
ENI_PRV_PADDR(skb) = paddr;
/* prepare DMA queue entries */
j = 0;
@@ -1226,8 +1226,8 @@ static void dequeue_tx(struct atm_dev *dev)
break;
}
ENI_VCC(vcc)->txing -= ENI_PRV_SIZE(skb);
-   pci_unmap_single(eni_dev->pci_dev,ENI_PRV_PADDR(skb),skb->len,
-   PCI_DMA_TODEVICE);
+   
dma_unmap_single(_dev->pci_dev->dev,ENI_PRV_PADDR(skb),skb->len,
+DMA_TO_DEVICE);
if (vcc->pop) vcc->pop(vcc,skb);
else dev_kfree_skb_irq(skb);
atomic_inc(>stats->tx);
@@ -2240,13 +2240,18 @@ static int eni_init_one(struct pci_dev *pci_dev,
if (rc < 0)
goto out;
 
+   rc = dma_set_mask_and_coherent(_dev->dev, DMA_BIT_MASK(32));
+   if (rc < 0)
+   goto out;
+
rc = -ENOMEM;
eni_dev = kmalloc(sizeof(struct eni_dev), GFP_KERNEL);
if (!eni_dev)
goto err_disable;
 
zero = _dev->zero;
-   zero->addr = pci_alloc_consistent(pci_dev, ENI_ZEROES_SIZE, >dma);
+   zero->addr = dma_alloc_coherent(_dev->dev,
+   ENI_ZEROES_SIZE, >dma, 
GFP_KERNEL);
if (!zero->addr)
goto err_kfree;
 
@@ -2277,7 +2282,7 @@ err_eni_release:
 err_unregister:
atm_dev_deregister(dev);
 err_free_consistent:
-   pci_free_consistent(pci_dev, ENI_ZEROES_SIZE, zero->addr, zero->dma);
+   dma_free_coherent(_dev->dev, ENI_ZEROES_SIZE, zero->addr, 
zero->dma);
 err_kfree:
kfree(eni_dev);
 err_disable:
@@ -2302,7 +2307,7 @@ static void eni_remove_one(struct pci_dev *pdev)
 
eni_do_release(dev);
atm_dev_deregister(dev);
-   pci_free_consistent(pdev, ENI_ZEROES_SIZE, zero->addr, zero->dma);
+   dma_free_coherent(>dev, ENI_ZEROES_SIZE, zero->addr, zero->dma);
kfree(ed);
pci_disable_device(pdev);
 }
diff --git a/drivers/atm/fore200e.c b/drivers/atm/fore200e.c
index d5d9eaf..75dde90 100644
--- a

[net-next PATCH v3 1/1] atm: remove deprecated use of pci api

[chas williams - CONTRACTOR]

Signed-off-by: Chas Williams - CONTRACTOR c...@cmf.nrl.navy.mil
---
 drivers/atm/eni.c   |  33 +++--
 drivers/atm/fore200e.c  |  22 +
 drivers/atm/he.c| 125 +---
 drivers/atm/he.h|   4 +-
 drivers/atm/idt77252.c  | 107 ++---
 drivers/atm/iphase.c|  54 +++--
 drivers/atm/lanai.c |  14 ++
 drivers/atm/nicstar.c   |  60 +++
 drivers/atm/solos-pci.c |  26 +-
 drivers/atm/zatm.c  |  17 ---
 10 files changed, 243 insertions(+), 219 deletions(-)

diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index c7fab3e..6339efd 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -354,9 +354,9 @@ static int do_rx_dma(struct atm_vcc *vcc,struct sk_buff 
*skb,
eni_vcc = ENI_VCC(vcc);
paddr = 0; /* GCC, shut up */
if (skb) {
-   paddr = pci_map_single(eni_dev-pci_dev,skb-data,skb-len,
-   PCI_DMA_FROMDEVICE);
-   if (pci_dma_mapping_error(eni_dev-pci_dev, paddr))
+   paddr = 
dma_map_single(eni_dev-pci_dev-dev,skb-data,skb-len,
+  DMA_FROM_DEVICE);
+   if (dma_mapping_error(eni_dev-pci_dev-dev, paddr))
goto dma_map_error;
ENI_PRV_PADDR(skb) = paddr;
if (paddr  3)
@@ -481,8 +481,8 @@ rx_enqueued++;
 
 trouble:
if (paddr)
-   pci_unmap_single(eni_dev-pci_dev,paddr,skb-len,
-   PCI_DMA_FROMDEVICE);
+   dma_unmap_single(eni_dev-pci_dev-dev,paddr,skb-len,
+DMA_FROM_DEVICE);
 dma_map_error:
if (skb) dev_kfree_skb_irq(skb);
return -1;
@@ -758,8 +758,8 @@ rx_dequeued++;
}
eni_vcc-rxing--;
eni_vcc-rx_pos = ENI_PRV_POS(skb)  (eni_vcc-words-1);
-   pci_unmap_single(eni_dev-pci_dev,ENI_PRV_PADDR(skb),skb-len,
-   PCI_DMA_TODEVICE);
+   
dma_unmap_single(eni_dev-pci_dev-dev,ENI_PRV_PADDR(skb),skb-len,
+DMA_TO_DEVICE);
if (!skb-len) dev_kfree_skb_irq(skb);
else {
EVENT(pushing (len=%ld)\n,skb-len,0);
@@ -1112,8 +1112,8 @@ DPRINTK(iovcnt = %d\n,skb_shinfo(skb)-nr_frags);
vcc-dev-number);
return enq_jam;
}
-   paddr = pci_map_single(eni_dev-pci_dev,skb-data,skb-len,
-   PCI_DMA_TODEVICE);
+   paddr = dma_map_single(eni_dev-pci_dev-dev,skb-data,skb-len,
+  DMA_TO_DEVICE);
ENI_PRV_PADDR(skb) = paddr;
/* prepare DMA queue entries */
j = 0;
@@ -1226,8 +1226,8 @@ static void dequeue_tx(struct atm_dev *dev)
break;
}
ENI_VCC(vcc)-txing -= ENI_PRV_SIZE(skb);
-   pci_unmap_single(eni_dev-pci_dev,ENI_PRV_PADDR(skb),skb-len,
-   PCI_DMA_TODEVICE);
+   
dma_unmap_single(eni_dev-pci_dev-dev,ENI_PRV_PADDR(skb),skb-len,
+DMA_TO_DEVICE);
if (vcc-pop) vcc-pop(vcc,skb);
else dev_kfree_skb_irq(skb);
atomic_inc(vcc-stats-tx);
@@ -2240,13 +2240,18 @@ static int eni_init_one(struct pci_dev *pci_dev,
if (rc  0)
goto out;
 
+   rc = dma_set_mask_and_coherent(pci_dev-dev, DMA_BIT_MASK(32));
+   if (rc  0)
+   goto out;
+
rc = -ENOMEM;
eni_dev = kmalloc(sizeof(struct eni_dev), GFP_KERNEL);
if (!eni_dev)
goto err_disable;
 
zero = eni_dev-zero;
-   zero-addr = pci_alloc_consistent(pci_dev, ENI_ZEROES_SIZE, zero-dma);
+   zero-addr = dma_alloc_coherent(pci_dev-dev,
+   ENI_ZEROES_SIZE, zero-dma, 
GFP_KERNEL);
if (!zero-addr)
goto err_kfree;
 
@@ -2277,7 +2282,7 @@ err_eni_release:
 err_unregister:
atm_dev_deregister(dev);
 err_free_consistent:
-   pci_free_consistent(pci_dev, ENI_ZEROES_SIZE, zero-addr, zero-dma);
+   dma_free_coherent(pci_dev-dev, ENI_ZEROES_SIZE, zero-addr, 
zero-dma);
 err_kfree:
kfree(eni_dev);
 err_disable:
@@ -2302,7 +2307,7 @@ static void eni_remove_one(struct pci_dev *pdev)
 
eni_do_release(dev);
atm_dev_deregister(dev);
-   pci_free_consistent(pdev, ENI_ZEROES_SIZE, zero-addr, zero-dma);
+   dma_free_coherent(pdev-dev, ENI_ZEROES_SIZE, zero-addr, zero-dma);
kfree(ed);
pci_disable_device(pdev);
 }
diff --git a/drivers/atm/fore200e.c b/drivers/atm/fore200e.c
index d5d9eaf..75dde90 100644
--- a/drivers/atm/fore200e.c
+++ b/drivers/atm/fore200e.c
@@ -425,7 +425,7 @@ static void fore200e_pca_write(u32 val, volatile u32 
__iomem *addr)
 static u32
 fore200e_pca_dma_map(struct fore200e* fore200e, void* virt_addr, int

Re: [net-next PATCH v3 1/1] atm: remove deprecated use of pci api

[chas williams - CONTRACTOR]

On Fri, 16 Jan 2015 15:10:25 +0100
Quentin Lambert lambert.quen...@gmail.com wrote:


  -u32 dma_addr = pci_map_single((struct pci_dev*)fore200e-bus_dev, 
  virt_addr, size, direction);
  +u32 dma_addr = dma_map_single(((struct pci_dev *) 
  fore200e-bus_dev)-dev, virt_addr, size, direction);

DPRINTK(3, PCI DVMA mapping: virt_addr = 0x%p, size = %d, direction 
  = %d,  -- dma_addr = 0x%08x\n,
  virt_addr, size, direction, dma_addr);
 
 []
 
 I am going try to make similar changes in some other part of the kernel and
 I was wondering if you could explain how you decided it wasn't necessary to
 check for ((struct pci_dev *) fore200e-bus_dev nullity for instance.

This gets set up in fore200e_pca_detect() which is pretty early in the
intialization process.  We don't get as far as using any of the DVMA
stubs unless pci_enable_device() succeeds, meaning pci_dev is good, and
fore200e-bus_dev is assigned to pci_dev (around line 2724).

fore200e-bus_dev is never cleared back to NULL, but obviously you
shouldn't be using any of the DMA routines after disabling the pci
device.  Hopefully the driver shuts down in an orderly fashion such
that all DMA is over by the time the driver disables the pci device.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] atm: remove deprecated use of pci api

[chas williams - CONTRACTOR]

On Tue, 13 Jan 2015 21:59:44 -0500 (EST)
David Miller  wrote:

> From: Quentin Lambert 
> Date: Mon, 12 Jan 2015 17:10:42 +0100
> 
> > @@ -2246,7 +2246,8 @@ static int eni_init_one(struct pci_dev *pci_dev,
> > goto err_disable;
> >  
> > zero = _dev->zero;
> > -   zero->addr = pci_alloc_consistent(pci_dev, ENI_ZEROES_SIZE, >dma);
> > +   zero->addr = dma_alloc_coherent(_dev->dev, ENI_ZEROES_SIZE,
> > +   >dma, GFP_ATOMIC);
> > if (!zero->addr)
> > goto err_kfree;
> >  
> 
> I really would like you to look at these locations and see if
> GFP_KERNEL can be used instead of GFP_ATOMIC.  I bet that nearly
> all of these can, and it is preferred.
> 
> Thanks.

I think I would like to go through and just fix all the usages of the
older pci interface.  This patch isn't very complete due to its
automated nature.

I will make some time this weekend.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] atm: remove deprecated use of pci api

[chas williams - CONTRACTOR]

On Tue, 13 Jan 2015 21:59:44 -0500 (EST)
David Miller da...@davemloft.net wrote:

 From: Quentin Lambert lambert.quen...@gmail.com
 Date: Mon, 12 Jan 2015 17:10:42 +0100
 
  @@ -2246,7 +2246,8 @@ static int eni_init_one(struct pci_dev *pci_dev,
  goto err_disable;
   
  zero = eni_dev-zero;
  -   zero-addr = pci_alloc_consistent(pci_dev, ENI_ZEROES_SIZE, zero-dma);
  +   zero-addr = dma_alloc_coherent(pci_dev-dev, ENI_ZEROES_SIZE,
  +   zero-dma, GFP_ATOMIC);
  if (!zero-addr)
  goto err_kfree;
   
 
 I really would like you to look at these locations and see if
 GFP_KERNEL can be used instead of GFP_ATOMIC.  I bet that nearly
 all of these can, and it is preferred.
 
 Thanks.

I think I would like to go through and just fix all the usages of the
older pci interface.  This patch isn't very complete due to its
automated nature.

I will make some time this weekend.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] moving from pci to dma

[chas williams - CONTRACTOR]

On Mon, 12 Jan 2015 16:32:46 +0100
Quentin Lambert  wrote:

> 
> On 12/01/2015 16:27, chas williams - CONTRACTOR wrote:
> > There doesn't seem to be a patch for review?
> >
> I made a mistake and forgot to number the mails. I did send them though.
> Would you like me to send them again, with the proper subject format?
> 
> 
> Quentin Lambert
> 

That would be nice.  Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] moving from pci to dma

[chas williams - CONTRACTOR]

There doesn't seem to be a patch for review?

On Mon, 12 Jan 2015 11:02:39 +0100
Quentin Lambert  wrote:

> This patch replaces the references to the deprecated pci api with the
> corresponding dma api.
...
> Quentin Lambert (1):
>   atm: remove deprecated use of pci api
> 
>  drivers/atm/eni.c   | 8 +---
>  drivers/atm/he.c| 2 +-
>  drivers/atm/lanai.c | 9 +
>  drivers/atm/nicstar.c   | 4 ++--
>  drivers/atm/solos-pci.c | 2 +-
>  drivers/atm/zatm.c  | 8 +---
>  6 files changed, 19 insertions(+), 14 deletions(-)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] moving from pci to dma

[chas williams - CONTRACTOR]

On Mon, 12 Jan 2015 16:32:46 +0100
Quentin Lambert lambert.quen...@gmail.com wrote:

 
 On 12/01/2015 16:27, chas williams - CONTRACTOR wrote:
  There doesn't seem to be a patch for review?
 
 I made a mistake and forgot to number the mails. I did send them though.
 Would you like me to send them again, with the proper subject format?
 
 
 Quentin Lambert
 

That would be nice.  Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] moving from pci to dma

[chas williams - CONTRACTOR]

There doesn't seem to be a patch for review?

On Mon, 12 Jan 2015 11:02:39 +0100
Quentin Lambert lambert.quen...@gmail.com wrote:

 This patch replaces the references to the deprecated pci api with the
 corresponding dma api.
...
 Quentin Lambert (1):
   atm: remove deprecated use of pci api
 
  drivers/atm/eni.c   | 8 +---
  drivers/atm/he.c| 2 +-
  drivers/atm/lanai.c | 9 +
  drivers/atm/nicstar.c   | 4 ++--
  drivers/atm/solos-pci.c | 2 +-
  drivers/atm/zatm.c  | 8 +---
  6 files changed, 19 insertions(+), 14 deletions(-)
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers:atm Remove two FIXMES in the function, top_off_fp for the file, firestream.c

[chas williams - CONTRACTOR]

I don't see any reason to promote qe_tmp to a u64.  I think you can
just remove the comment.  Anyone trying to build this into a 64-bit
kernel will see errors from the virt_to_bus()/bus_to_virt() usage.

fp->n seems to only be manipulated in interrupt context (after driving
initialization) so it doesn't need locking or to be atomic.

On Sat,  6 Dec 2014 22:35:48 -0500
Nicholas Krause  wrote:

> Removes two FIXMES in the function.top_off_fp. The first being that of 
> needing the variable, qe_tmp needing to be a
> u64 type and not u32 as encoding will not work if using a 32 bit register 
> rather then 64 bit as stated in the first
> fix me comment. In addition the second being a no longer needed comment due 
> to not needing to atomically increment
> the variable, n as passed to the function by the pointer of fp, as part of a 
> structure of type,freepool.
> 
> Signed-off-by: Nicholas Krause 
> ---
>  drivers/atm/firestream.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/atm/firestream.c b/drivers/atm/firestream.c
> index 82f2ae0..06c23f6 100644
> --- a/drivers/atm/firestream.c
> +++ b/drivers/atm/firestream.c
> @@ -1477,7 +1477,7 @@ static void top_off_fp (struct fs_dev *dev, struct 
> freepool *fp,
>   struct FS_BPENTRY *qe, *ne;
>   struct sk_buff *skb;
>   int n = 0;
> - u32 qe_tmp;
> + u64  qe_tmp;
>  
>   fs_dprintk (FS_DEBUG_QUEUE, "Topping off queue at %x (%d-%d/%d)\n", 
>   fp->offset, read_fs (dev, FP_CNT (fp->offset)), fp->n, 
> @@ -1505,14 +1505,8 @@ static void top_off_fp (struct fs_dev *dev, struct 
> freepool *fp,
>   ne->skb = skb;
>   ne->fp = fp;
>  
> - /*
> -  * FIXME: following code encodes and decodes
> -  * machine pointers (could be 64-bit) into a
> -  * 32-bit register.
> -  */
> -
>   qe_tmp = read_fs (dev, FP_EA(fp->offset));
> - fs_dprintk (FS_DEBUG_QUEUE, "link at %x\n", qe_tmp);
> + fs_dprintk(FS_DEBUG_QUEUE, "link at %llx\n", qe_tmp);
>   if (qe_tmp) {
>   qe = bus_to_virt ((long) qe_tmp);
>   qe->next = virt_to_bus(ne);
> @@ -1521,7 +1515,7 @@ static void top_off_fp (struct fs_dev *dev, struct 
> freepool *fp,
>   write_fs (dev, FP_SA(fp->offset), virt_to_bus(ne));
>  
>   write_fs (dev, FP_EA(fp->offset), virt_to_bus (ne));
> - fp->n++;   /* XXX Atomic_inc? */
> + fp->n++;
>   write_fs (dev, FP_CTU(fp->offset), 1);
>   }
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers:atm Remove two FIXMES in the function, top_off_fp for the file, firestream.c

[chas williams - CONTRACTOR]

I don't see any reason to promote qe_tmp to a u64.  I think you can
just remove the comment.  Anyone trying to build this into a 64-bit
kernel will see errors from the virt_to_bus()/bus_to_virt() usage.

fp-n seems to only be manipulated in interrupt context (after driving
initialization) so it doesn't need locking or to be atomic.

On Sat,  6 Dec 2014 22:35:48 -0500
Nicholas Krause xerofo...@gmail.com wrote:

 Removes two FIXMES in the function.top_off_fp. The first being that of 
 needing the variable, qe_tmp needing to be a
 u64 type and not u32 as encoding will not work if using a 32 bit register 
 rather then 64 bit as stated in the first
 fix me comment. In addition the second being a no longer needed comment due 
 to not needing to atomically increment
 the variable, n as passed to the function by the pointer of fp, as part of a 
 structure of type,freepool.
 
 Signed-off-by: Nicholas Krause xerofo...@gmail.com
 ---
  drivers/atm/firestream.c | 12 +++-
  1 file changed, 3 insertions(+), 9 deletions(-)
 
 diff --git a/drivers/atm/firestream.c b/drivers/atm/firestream.c
 index 82f2ae0..06c23f6 100644
 --- a/drivers/atm/firestream.c
 +++ b/drivers/atm/firestream.c
 @@ -1477,7 +1477,7 @@ static void top_off_fp (struct fs_dev *dev, struct 
 freepool *fp,
   struct FS_BPENTRY *qe, *ne;
   struct sk_buff *skb;
   int n = 0;
 - u32 qe_tmp;
 + u64  qe_tmp;
  
   fs_dprintk (FS_DEBUG_QUEUE, Topping off queue at %x (%d-%d/%d)\n, 
   fp-offset, read_fs (dev, FP_CNT (fp-offset)), fp-n, 
 @@ -1505,14 +1505,8 @@ static void top_off_fp (struct fs_dev *dev, struct 
 freepool *fp,
   ne-skb = skb;
   ne-fp = fp;
  
 - /*
 -  * FIXME: following code encodes and decodes
 -  * machine pointers (could be 64-bit) into a
 -  * 32-bit register.
 -  */
 -
   qe_tmp = read_fs (dev, FP_EA(fp-offset));
 - fs_dprintk (FS_DEBUG_QUEUE, link at %x\n, qe_tmp);
 + fs_dprintk(FS_DEBUG_QUEUE, link at %llx\n, qe_tmp);
   if (qe_tmp) {
   qe = bus_to_virt ((long) qe_tmp);
   qe-next = virt_to_bus(ne);
 @@ -1521,7 +1515,7 @@ static void top_off_fp (struct fs_dev *dev, struct 
 freepool *fp,
   write_fs (dev, FP_SA(fp-offset), virt_to_bus(ne));
  
   write_fs (dev, FP_EA(fp-offset), virt_to_bus (ne));
 - fp-n++;   /* XXX Atomic_inc? */
 + fp-n++;
   write_fs (dev, FP_CTU(fp-offset), 1);
   }
  

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: FIXME in solos-pci.c

[chas williams - CONTRACTOR]

The last I heard on this topic was from Guy Ellis,

From: Guy Ellis 
To: linux-atm-gene...@lists.sourceforge.net
Subject: Re: [Linux-ATM-General] solos-pci.c: Fix me
Date: Tue, 22 Jul 2014 07:34:30 +1000

Hi Chas/Nick,

I think the FIXME is reminder to deal correctly with an unknown command.
At the moment an unknown command is treated as a PKT_COMMAND which is 
incorrect.
I think the correct behaviour would be to ignore unknown commands and 
just free the skb.

That said I doubt this condition ever occurs, which is probably why it 
has been this way since day 1.

Nathan is on vacation at the moment, when he gets back in August I'll 
ask him to tidy this up.

So perhaps, Guy could let us know if he had a chance for Nathan to
look at removing this FIXME.


On Wed, 03 Dec 2014 22:58:46 -0500
nick  wrote:

> Greetings Chas,
> I am wondering if there is any reason for the FIXME message in the below 
> code. This is due to after reading the other parts of the function the code 
> and logical seem similar and correct unless I am missing something about the 
> hardware.
> Cheers Nick 
>  case PKT_COMMAND:
>default: /* FIXME: Not really, surely? */
>   if (process_command(card, port, skb))
> break;
>spin_lock(>cli_queue_lock);
>if (skb_queue_len(>cli_queue[port]) > 
> 10) {
>  if (net_ratelimit())
> dev_warn(>dev->dev, 
> "Dropping console response on port %d\n",
>  port);
> dev_kfree_skb_any(skb);
> } else
>  
> skb_queue_tail(>cli_queue[port], skb);
>spin_unlock(>cli_queue_lock);
>break;
>}
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: FIXME in solos-pci.c

[chas williams - CONTRACTOR]

The last I heard on this topic was from Guy Ellis,

From: Guy Ellis g...@traverse.com.au
To: linux-atm-gene...@lists.sourceforge.net
Subject: Re: [Linux-ATM-General] solos-pci.c: Fix me
Date: Tue, 22 Jul 2014 07:34:30 +1000

Hi Chas/Nick,

I think the FIXME is reminder to deal correctly with an unknown command.
At the moment an unknown command is treated as a PKT_COMMAND which is 
incorrect.
I think the correct behaviour would be to ignore unknown commands and 
just free the skb.

That said I doubt this condition ever occurs, which is probably why it 
has been this way since day 1.

Nathan is on vacation at the moment, when he gets back in August I'll 
ask him to tidy this up.

So perhaps, Guy could let us know if he had a chance for Nathan to
look at removing this FIXME.


On Wed, 03 Dec 2014 22:58:46 -0500
nick xerofo...@gmail.com wrote:

 Greetings Chas,
 I am wondering if there is any reason for the FIXME message in the below 
 code. This is due to after reading the other parts of the function the code 
 and logical seem similar and correct unless I am missing something about the 
 hardware.
 Cheers Nick 
  case PKT_COMMAND:
default: /* FIXME: Not really, surely? */
   if (process_command(card, port, skb))
 break;
spin_lock(card-cli_queue_lock);
if (skb_queue_len(card-cli_queue[port])  
 10) {
  if (net_ratelimit())
 dev_warn(card-dev-dev, 
 Dropping console response on port %d\n,
  port);
 dev_kfree_skb_any(skb);
 } else
  
 skb_queue_tail(card-cli_queue[port], skb);
spin_unlock(card-cli_queue_lock);
break;
}
  
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] atm/svc: Fix blocking in wait loop

[chas williams - CONTRACTOR]

One should not call blocking primitives inside a wait loop, since both
require task_struct::state to sleep, so the inner will destroy the
outer state.

sigd_enq() will possibly sleep for alloc_skb().  Move sigd_enq() before
prepare_to_wait() to avoid sleeping while waiting interruptibly.  You do
not actually need to call sigd_enq() after the initial prepare_to_wait()
because we test the termination condition before calling schedule().

Based on suggestions from Peter Zijlstra.

Signed-off-by: Chas Williams 
Acked-by: Peter Zijlstra 
---
 net/atm/svc.c | 60 +++
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/net/atm/svc.c b/net/atm/svc.c
index d8e5d0c2..1ba23f5 100644
--- a/net/atm/svc.c
+++ b/net/atm/svc.c
@@ -50,12 +50,12 @@ static void svc_disconnect(struct atm_vcc *vcc)
 
pr_debug("%p\n", vcc);
if (test_bit(ATM_VF_REGIS, >flags)) {
-   prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);
sigd_enq(vcc, as_close, NULL, NULL, NULL);
-   while (!test_bit(ATM_VF_RELEASED, >flags) && sigd) {
+   for (;;) {
+   prepare_to_wait(sk_sleep(sk), , 
TASK_UNINTERRUPTIBLE);
+   if (test_bit(ATM_VF_RELEASED, >flags) || !sigd)
+   break;
schedule();
-   prepare_to_wait(sk_sleep(sk), ,
-   TASK_UNINTERRUPTIBLE);
}
finish_wait(sk_sleep(sk), );
}
@@ -126,11 +126,12 @@ static int svc_bind(struct socket *sock, struct sockaddr 
*sockaddr,
}
vcc->local = *addr;
set_bit(ATM_VF_WAITING, >flags);
-   prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);
sigd_enq(vcc, as_bind, NULL, NULL, >local);
-   while (test_bit(ATM_VF_WAITING, >flags) && sigd) {
-   schedule();
+   for (;;) {
prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);
+   if (!test_bit(ATM_VF_WAITING, >flags) || !sigd)
+   break;
+   schedule();
}
finish_wait(sk_sleep(sk), );
clear_bit(ATM_VF_REGIS, >flags); /* doesn't count */
@@ -202,15 +203,14 @@ static int svc_connect(struct socket *sock, struct 
sockaddr *sockaddr,
}
vcc->remote = *addr;
set_bit(ATM_VF_WAITING, >flags);
-   prepare_to_wait(sk_sleep(sk), , TASK_INTERRUPTIBLE);
sigd_enq(vcc, as_connect, NULL, NULL, >remote);
if (flags & O_NONBLOCK) {
-   finish_wait(sk_sleep(sk), );
sock->state = SS_CONNECTING;
error = -EINPROGRESS;
goto out;
}
error = 0;
+   prepare_to_wait(sk_sleep(sk), , TASK_INTERRUPTIBLE);
while (test_bit(ATM_VF_WAITING, >flags) && sigd) {
schedule();
if (!signal_pending(current)) {
@@ -297,11 +297,12 @@ static int svc_listen(struct socket *sock, int backlog)
goto out;
}
set_bit(ATM_VF_WAITING, >flags);
-   prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);
sigd_enq(vcc, as_listen, NULL, NULL, >local);
-   while (test_bit(ATM_VF_WAITING, >flags) && sigd) {
-   schedule();
+   for (;;) {
prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);
+   if (!test_bit(ATM_VF_WAITING, >flags) || !sigd)
+   break;
+   schedule();
}
finish_wait(sk_sleep(sk), );
if (!sigd) {
@@ -387,15 +388,15 @@ static int svc_accept(struct socket *sock, struct socket 
*newsock, int flags)
}
/* wait should be short, so we ignore the non-blocking flag */
set_bit(ATM_VF_WAITING, _vcc->flags);
-   prepare_to_wait(sk_sleep(sk_atm(new_vcc)), ,
-   TASK_UNINTERRUPTIBLE);
sigd_enq(new_vcc, as_accept, old_vcc, NULL, NULL);
-   while (test_bit(ATM_VF_WAITING, _vcc->flags) && sigd) {
+   for (;;) {
+   prepare_to_wait(sk_sleep(sk_atm(new_vcc)), ,
+   TASK_UNINTERRUPTIBLE);
+   if (!test_bit(ATM_VF_WAITING, _vcc->flags) || !sigd)
+   break;
release_sock(sk);
schedule();
lock_sock(sk);
-   prepare_to_wait(sk_sleep(sk_atm(new_vcc)), ,
-   TASK_UNINTERRUPTIBLE);
}
finish_wait(sk_sleep(sk_atm(new_vcc)), );
if (

[PATCH] atm/svc: Fix blocking in wait loop

[chas williams - CONTRACTOR]

One should not call blocking primitives inside a wait loop, since both
require task_struct::state to sleep, so the inner will destroy the
outer state.

sigd_enq() will possibly sleep for alloc_skb().  Move sigd_enq() before
prepare_to_wait() to avoid sleeping while waiting interruptibly.  You do
not actually need to call sigd_enq() after the initial prepare_to_wait()
because we test the termination condition before calling schedule().

Based on suggestions from Peter Zijlstra.

Signed-off-by: Chas Williams c...@cmf.n4rl.navy.mil
Acked-by: Peter Zijlstra pet...@infradead.org
---
 net/atm/svc.c | 60 +++
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/net/atm/svc.c b/net/atm/svc.c
index d8e5d0c2..1ba23f5 100644
--- a/net/atm/svc.c
+++ b/net/atm/svc.c
@@ -50,12 +50,12 @@ static void svc_disconnect(struct atm_vcc *vcc)
 
pr_debug(%p\n, vcc);
if (test_bit(ATM_VF_REGIS, vcc-flags)) {
-   prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);
sigd_enq(vcc, as_close, NULL, NULL, NULL);
-   while (!test_bit(ATM_VF_RELEASED, vcc-flags)  sigd) {
+   for (;;) {
+   prepare_to_wait(sk_sleep(sk), wait, 
TASK_UNINTERRUPTIBLE);
+   if (test_bit(ATM_VF_RELEASED, vcc-flags) || !sigd)
+   break;
schedule();
-   prepare_to_wait(sk_sleep(sk), wait,
-   TASK_UNINTERRUPTIBLE);
}
finish_wait(sk_sleep(sk), wait);
}
@@ -126,11 +126,12 @@ static int svc_bind(struct socket *sock, struct sockaddr 
*sockaddr,
}
vcc-local = *addr;
set_bit(ATM_VF_WAITING, vcc-flags);
-   prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);
sigd_enq(vcc, as_bind, NULL, NULL, vcc-local);
-   while (test_bit(ATM_VF_WAITING, vcc-flags)  sigd) {
-   schedule();
+   for (;;) {
prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);
+   if (!test_bit(ATM_VF_WAITING, vcc-flags) || !sigd)
+   break;
+   schedule();
}
finish_wait(sk_sleep(sk), wait);
clear_bit(ATM_VF_REGIS, vcc-flags); /* doesn't count */
@@ -202,15 +203,14 @@ static int svc_connect(struct socket *sock, struct 
sockaddr *sockaddr,
}
vcc-remote = *addr;
set_bit(ATM_VF_WAITING, vcc-flags);
-   prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
sigd_enq(vcc, as_connect, NULL, NULL, vcc-remote);
if (flags  O_NONBLOCK) {
-   finish_wait(sk_sleep(sk), wait);
sock-state = SS_CONNECTING;
error = -EINPROGRESS;
goto out;
}
error = 0;
+   prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
while (test_bit(ATM_VF_WAITING, vcc-flags)  sigd) {
schedule();
if (!signal_pending(current)) {
@@ -297,11 +297,12 @@ static int svc_listen(struct socket *sock, int backlog)
goto out;
}
set_bit(ATM_VF_WAITING, vcc-flags);
-   prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);
sigd_enq(vcc, as_listen, NULL, NULL, vcc-local);
-   while (test_bit(ATM_VF_WAITING, vcc-flags)  sigd) {
-   schedule();
+   for (;;) {
prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);
+   if (!test_bit(ATM_VF_WAITING, vcc-flags) || !sigd)
+   break;
+   schedule();
}
finish_wait(sk_sleep(sk), wait);
if (!sigd) {
@@ -387,15 +388,15 @@ static int svc_accept(struct socket *sock, struct socket 
*newsock, int flags)
}
/* wait should be short, so we ignore the non-blocking flag */
set_bit(ATM_VF_WAITING, new_vcc-flags);
-   prepare_to_wait(sk_sleep(sk_atm(new_vcc)), wait,
-   TASK_UNINTERRUPTIBLE);
sigd_enq(new_vcc, as_accept, old_vcc, NULL, NULL);
-   while (test_bit(ATM_VF_WAITING, new_vcc-flags)  sigd) {
+   for (;;) {
+   prepare_to_wait(sk_sleep(sk_atm(new_vcc)), wait,
+   TASK_UNINTERRUPTIBLE);
+   if (!test_bit(ATM_VF_WAITING, new_vcc-flags) || !sigd)
+   break;
release_sock(sk);
schedule();
lock_sock(sk);
-   prepare_to_wait(sk_sleep(sk_atm(new_vcc)), wait,
-   TASK_UNINTERRUPTIBLE);
}
finish_wait(sk_sleep(sk_atm(new_vcc)), wait

Re: [setsockopt] WARNING: CPU: 0 PID: 1444 at kernel/sched/core.c:7088 __might_sleep+0x51/0x16f()

[chas williams - CONTRACTOR]

On Thu, 7 Aug 2014 17:17:41 +0200
Peter Zijlstra  wrote:

> Subject: atm: Fix blocking in wait loop
> 
> One should not call blocking primitives inside a wait loop, since both
> require task_struct::state to sleep, so the inner will destroy the outer
> state.
> 
> In this instance sigd_enq() will possible sleep for alloc_skb(), now if
> I understand the code right, we do not actually need to call sigd_enq()
> after the initial prepare_to_wait(), because we test the termination
> condition before schedule() anyhow.
> 
> So we can simply move it up a bit and avoid the entire confusion.
> 
> Signed-off-by: Peter Zijlstra 
> ---
>  net/atm/svc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/atm/svc.c b/net/atm/svc.c
> index d8e5d0c2ebbc..445ac238b69b 100644
> --- a/net/atm/svc.c
> +++ b/net/atm/svc.c
> @@ -297,8 +297,8 @@ static int svc_listen(struct socket *sock, int backlog)
>   goto out;
>   }
>   set_bit(ATM_VF_WAITING, >flags);
> - prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);
>   sigd_enq(vcc, as_listen, NULL, NULL, >local);
> + prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);
>   while (test_bit(ATM_VF_WAITING, >flags) && sigd) {
>   schedule();
>   prepare_to_wait(sk_sleep(sk), , TASK_UNINTERRUPTIBLE);


This isn't the only place that we queue a message for the signalling
daemon after a prepare_to_wait() uninterruptibly so this patch would
be incomplete as is.

What bothers me is the TASK_UNINTERRUPTIBLE -- I don't have a good
reason why any of these should be sleeping uninterruptibly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] drivers/atm/atmtcp.c: fix error return code

[chas williams - CONTRACTOR]

On Thu, 7 Aug 2014 15:31:05 +0200 (CEST)
Julia Lawall  wrote:


> diff --git a/drivers/atm/atmtcp.c b/drivers/atm/atmtcp.c
> index 0e3f8f9..c8e4fb4 100644
> --- a/drivers/atm/atmtcp.c
> +++ b/drivers/atm/atmtcp.c
> @@ -299,6 +299,7 @@ static int atmtcp_c_send(struct atm_vcc *vcc,struct 
> sk_buff *skb)
>   out_vcc = find_vcc(dev, ntohs(hdr->vpi), ntohs(hdr->vci));
>   read_unlock(_sklist_lock);
>   if (!out_vcc) {
> + result = -EUNATCH;
>   atomic_inc(>stats->tx_err);
>   goto done;
>   }
> 

Acked-by: Chas Williams 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] solos-pci: fix error return code

[chas williams - CONTRACTOR]

On Thu,  7 Aug 2014 14:49:07 +0200
Julia Lawall  wrote:

...
>  drivers/atm/solos-pci.c |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
> index 943cf0d..7652e8d 100644
> --- a/drivers/atm/solos-pci.c
> +++ b/drivers/atm/solos-pci.c
> @@ -1278,6 +1278,7 @@ static int fpga_probe(struct pci_dev *dev, const struct 
> pci_device_id *id)
>   card->dma_bounce = kmalloc(card->nr_ports * BUF_SIZE, 
> GFP_KERNEL);
>   if (!card->dma_bounce) {
>   dev_warn(>dev->dev, "Failed to allocate 
> DMA bounce buffers\n");
> + err = -ENOMEM;
>   /* Fallback to MMIO doesn't work */
>   goto out_unmap_both;
>   }
> 

Acked-by: Chas Williams 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] drivers/atm/atmtcp.c: fix error return code

[chas williams - CONTRACTOR]

On Thu,  7 Aug 2014 14:49:06 +0200
Julia Lawall  wrote:

> From: Julia Lawall 
> 
> Convert a zero return value on error to a negative one, as returned
> elsewhere in the function.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
...
> 
> diff --git a/drivers/atm/atmtcp.c b/drivers/atm/atmtcp.c
> index 0e3f8f9..c8e4fb4 100644
> --- a/drivers/atm/atmtcp.c
> +++ b/drivers/atm/atmtcp.c
> @@ -299,6 +299,7 @@ static int atmtcp_c_send(struct atm_vcc *vcc,struct 
> sk_buff *skb)
>   out_vcc = find_vcc(dev, ntohs(hdr->vpi), ntohs(hdr->vci));
>   read_unlock(_sklist_lock);
>   if (!out_vcc) {
> + result = -ESRCH;

This should probably be -EUNATCH to match the rest of the code.

>   atomic_inc(>stats->tx_err);
>   goto done;
>   }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] drivers/atm/atmtcp.c: fix error return code

[chas williams - CONTRACTOR]

On Thu,  7 Aug 2014 14:49:06 +0200
Julia Lawall julia.law...@lip6.fr wrote:

 From: Julia Lawall julia.law...@lip6.fr
 
 Convert a zero return value on error to a negative one, as returned
 elsewhere in the function.
 
 A simplified version of the semantic match that finds this problem is as
 follows: (http://coccinelle.lip6.fr/)
...
 
 diff --git a/drivers/atm/atmtcp.c b/drivers/atm/atmtcp.c
 index 0e3f8f9..c8e4fb4 100644
 --- a/drivers/atm/atmtcp.c
 +++ b/drivers/atm/atmtcp.c
 @@ -299,6 +299,7 @@ static int atmtcp_c_send(struct atm_vcc *vcc,struct 
 sk_buff *skb)
   out_vcc = find_vcc(dev, ntohs(hdr-vpi), ntohs(hdr-vci));
   read_unlock(vcc_sklist_lock);
   if (!out_vcc) {
 + result = -ESRCH;

This should probably be -EUNATCH to match the rest of the code.

   atomic_inc(vcc-stats-tx_err);
   goto done;
   }
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] solos-pci: fix error return code

[chas williams - CONTRACTOR]

On Thu,  7 Aug 2014 14:49:07 +0200
Julia Lawall julia.law...@lip6.fr wrote:

...
  drivers/atm/solos-pci.c |1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
 index 943cf0d..7652e8d 100644
 --- a/drivers/atm/solos-pci.c
 +++ b/drivers/atm/solos-pci.c
 @@ -1278,6 +1278,7 @@ static int fpga_probe(struct pci_dev *dev, const struct 
 pci_device_id *id)
   card-dma_bounce = kmalloc(card-nr_ports * BUF_SIZE, 
 GFP_KERNEL);
   if (!card-dma_bounce) {
   dev_warn(card-dev-dev, Failed to allocate 
 DMA bounce buffers\n);
 + err = -ENOMEM;
   /* Fallback to MMIO doesn't work */
   goto out_unmap_both;
   }
 

Acked-by: Chas Williams c...@cmf.nrl.navy.mil
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] drivers/atm/atmtcp.c: fix error return code

[chas williams - CONTRACTOR]

On Thu, 7 Aug 2014 15:31:05 +0200 (CEST)
Julia Lawall julia.law...@lip6.fr wrote:


 diff --git a/drivers/atm/atmtcp.c b/drivers/atm/atmtcp.c
 index 0e3f8f9..c8e4fb4 100644
 --- a/drivers/atm/atmtcp.c
 +++ b/drivers/atm/atmtcp.c
 @@ -299,6 +299,7 @@ static int atmtcp_c_send(struct atm_vcc *vcc,struct 
 sk_buff *skb)
   out_vcc = find_vcc(dev, ntohs(hdr-vpi), ntohs(hdr-vci));
   read_unlock(vcc_sklist_lock);
   if (!out_vcc) {
 + result = -EUNATCH;
   atomic_inc(vcc-stats-tx_err);
   goto done;
   }
 

Acked-by: Chas Williams c...@cmf.nrl.navy.mil
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [setsockopt] WARNING: CPU: 0 PID: 1444 at kernel/sched/core.c:7088 __might_sleep+0x51/0x16f()

[chas williams - CONTRACTOR]

On Thu, 7 Aug 2014 17:17:41 +0200
Peter Zijlstra pet...@infradead.org wrote:

 Subject: atm: Fix blocking in wait loop
 
 One should not call blocking primitives inside a wait loop, since both
 require task_struct::state to sleep, so the inner will destroy the outer
 state.
 
 In this instance sigd_enq() will possible sleep for alloc_skb(), now if
 I understand the code right, we do not actually need to call sigd_enq()
 after the initial prepare_to_wait(), because we test the termination
 condition before schedule() anyhow.
 
 So we can simply move it up a bit and avoid the entire confusion.
 
 Signed-off-by: Peter Zijlstra pet...@infradead.org
 ---
  net/atm/svc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/net/atm/svc.c b/net/atm/svc.c
 index d8e5d0c2ebbc..445ac238b69b 100644
 --- a/net/atm/svc.c
 +++ b/net/atm/svc.c
 @@ -297,8 +297,8 @@ static int svc_listen(struct socket *sock, int backlog)
   goto out;
   }
   set_bit(ATM_VF_WAITING, vcc-flags);
 - prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);
   sigd_enq(vcc, as_listen, NULL, NULL, vcc-local);
 + prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);
   while (test_bit(ATM_VF_WAITING, vcc-flags)  sigd) {
   schedule();
   prepare_to_wait(sk_sleep(sk), wait, TASK_UNINTERRUPTIBLE);


This isn't the only place that we queue a message for the signalling
daemon after a prepare_to_wait() uninterruptibly so this patch would
be incomplete as is.

What bothers me is the TASK_UNINTERRUPTIBLE -- I don't have a good
reason why any of these should be sleeping uninterruptibly.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] drivers: atm: fix %d confusingly prefixed with 0x in format strings

[chas williams - CONTRACTOR]

Acked-by: Chas Williams 

On Sun,  3 Aug 2014 17:18:20 -0700
Hans Wennborg  wrote:

> Signed-off-by: Hans Wennborg 
> ---
>  drivers/atm/eni.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
> index b1955ba..d65975a 100644
> --- a/drivers/atm/eni.c
> +++ b/drivers/atm/eni.c
> @@ -2155,7 +2155,7 @@ static int eni_proc_read(struct atm_dev *dev,loff_t 
> *pos,char *page)
>  
>   if (!tx->send) continue;
>   if (!--left) {
> - return sprintf(page,"tx[%d]:0x%ld-0x%ld "
> + return sprintf(page, "tx[%d]:0x%lx-0x%lx "
>   "(%6ld bytes), rsv %d cps, shp %d cps%s\n",i,
>   (unsigned long) (tx->send - eni_dev->ram),
>   tx->send-eni_dev->ram+tx->words*4-1,tx->words*4,
> @@ -2181,7 +2181,7 @@ static int eni_proc_read(struct atm_dev *dev,loff_t 
> *pos,char *page)
>   if (--left) continue;
>   length = sprintf(page,"vcc %4d: ",vcc->vci);
>   if (eni_vcc->rx) {
> - length += sprintf(page+length,"0x%ld-0x%ld "
> + length += sprintf(page+length, "0x%lx-0x%lx "
>   "(%6ld bytes)",
>   (unsigned long) (eni_vcc->recv - 
> eni_dev->ram),
>   
> eni_vcc->recv-eni_dev->ram+eni_vcc->words*4-1,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] drivers: atm: fix %d confusingly prefixed with 0x in format strings

[chas williams - CONTRACTOR]

Acked-by: Chas Williams c...@cmf.nrl.navy.mil

On Sun,  3 Aug 2014 17:18:20 -0700
Hans Wennborg h...@hanshq.net wrote:

 Signed-off-by: Hans Wennborg h...@hanshq.net
 ---
  drivers/atm/eni.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
 index b1955ba..d65975a 100644
 --- a/drivers/atm/eni.c
 +++ b/drivers/atm/eni.c
 @@ -2155,7 +2155,7 @@ static int eni_proc_read(struct atm_dev *dev,loff_t 
 *pos,char *page)
  
   if (!tx-send) continue;
   if (!--left) {
 - return sprintf(page,tx[%d]:0x%ld-0x%ld 
 + return sprintf(page, tx[%d]:0x%lx-0x%lx 
   (%6ld bytes), rsv %d cps, shp %d cps%s\n,i,
   (unsigned long) (tx-send - eni_dev-ram),
   tx-send-eni_dev-ram+tx-words*4-1,tx-words*4,
 @@ -2181,7 +2181,7 @@ static int eni_proc_read(struct atm_dev *dev,loff_t 
 *pos,char *page)
   if (--left) continue;
   length = sprintf(page,vcc %4d: ,vcc-vci);
   if (eni_vcc-rx) {
 - length += sprintf(page+length,0x%ld-0x%ld 
 + length += sprintf(page+length, 0x%lx-0x%lx 
   (%6ld bytes),
   (unsigned long) (eni_vcc-recv - 
 eni_dev-ram),
   
 eni_vcc-recv-eni_dev-ram+eni_vcc-words*4-1,

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: solos-pci.c: Fix me

[chas williams - CONTRACTOR]

On Mon, 21 Jul 2014 13:42:01 -0400
Nick Krause  wrote:

> On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR
>  wrote:
...
> > @@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
> > dev_kfree_skb_any(skb);
> > break;
> >
> > -   case PKT_COMMAND:
> > -   default: /* FIXME: Not really, surely? */
> > +   default: /* PKT_COMMAND */
> > if (process_command(card, port, skb))
> > break;power
> > spin_lock(>cli_queue_lock);
> >
> > and be done with it since that will preserve existing behavior.
> 
> 
> My only question then is this function causing bugs as is?
> Cheers Nick
> 

It has been there since the first version of this driver was released.
If it were breaking things, I would have to guess it would have been
noticed by now.

Just looking at the PKT_* defines, I would wildly guess that PKT_COMMAND
PKT_POPEN and PKT_PCLOSE are all being handled via the PKT_COMMAND|default
case.  If not, popen and pclose would be leaking a skb for each usage
(which would be opening and closing the PVC -- not very often).

popen and pclose just seem like specialized PKT_COMMAND types.
process_command() woud ignore PKT_POPEN and PKT_PCLOSE since they aren't
long enough to contain a command.  So maybe the comment should just go
away.

If you had access to hardware you could probably figure this out pretty
quickly.  The programmer's manual doesn't seem to be available.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: solos-pci.c: Fix me

[chas williams - CONTRACTOR]

On Sun, 20 Jul 2014 00:59:42 -0400
Nick Krause  wrote:

> Hey Chas,
> There seems to be a fix me in this file in the function, solos_bh.
> Is the default statement correct and I remove the fix me or
> does it need to be rewritten.
> Cheers Nick
> 

I am afraid that I don't know enough about the solos hardware to know
if it needs to be rewritten.  I gather the solos returns at least three
kinds of packets, data, status and command.  If you wish to eliminate
the FIXME comment, you could just write:

@@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
dev_kfree_skb_any(skb);
break;
 
-   case PKT_COMMAND:
-   default: /* FIXME: Not really, surely? */
+   default: /* PKT_COMMAND */
if (process_command(card, port, skb))
break;
spin_lock(>cli_queue_lock);

and be done with it since that will preserve existing behavior.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: solos-pci.c: Fix me

[chas williams - CONTRACTOR]

On Sun, 20 Jul 2014 00:59:42 -0400
Nick Krause xerofo...@gmail.com wrote:

 Hey Chas,
 There seems to be a fix me in this file in the function, solos_bh.
 Is the default statement correct and I remove the fix me or
 does it need to be rewritten.
 Cheers Nick
 

I am afraid that I don't know enough about the solos hardware to know
if it needs to be rewritten.  I gather the solos returns at least three
kinds of packets, data, status and command.  If you wish to eliminate
the FIXME comment, you could just write:

@@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
dev_kfree_skb_any(skb);
break;
 
-   case PKT_COMMAND:
-   default: /* FIXME: Not really, surely? */
+   default: /* PKT_COMMAND */
if (process_command(card, port, skb))
break;
spin_lock(card-cli_queue_lock);

and be done with it since that will preserve existing behavior.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: solos-pci.c: Fix me

[chas williams - CONTRACTOR]

On Mon, 21 Jul 2014 13:42:01 -0400
Nick Krause xerofo...@gmail.com wrote:

 On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR
 c...@cmf.nrl.navy.mil wrote:
...
  @@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
  dev_kfree_skb_any(skb);
  break;
 
  -   case PKT_COMMAND:
  -   default: /* FIXME: Not really, surely? */
  +   default: /* PKT_COMMAND */
  if (process_command(card, port, skb))
  break;power
  spin_lock(card-cli_queue_lock);
 
  and be done with it since that will preserve existing behavior.
 
 
 My only question then is this function causing bugs as is?
 Cheers Nick
 

It has been there since the first version of this driver was released.
If it were breaking things, I would have to guess it would have been
noticed by now.

Just looking at the PKT_* defines, I would wildly guess that PKT_COMMAND
PKT_POPEN and PKT_PCLOSE are all being handled via the PKT_COMMAND|default
case.  If not, popen and pclose would be leaking a skb for each usage
(which would be opening and closing the PVC -- not very often).

popen and pclose just seem like specialized PKT_COMMAND types.
process_command() woud ignore PKT_POPEN and PKT_PCLOSE since they aren't
long enough to contain a command.  So maybe the comment should just go
away.

If you had access to hardware you could probably figure this out pretty
quickly.  The programmer's manual doesn't seem to be available.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: solos-pci: make solos_bh() as static

[chas williams - CONTRACTOR]

On Wed, 19 Feb 2014 14:13:54 +0900
Daeseok Youn  wrote:

> >From 6297aabeff748777b520cc0ee835af0a3ddc79e2 Mon Sep 17 00:00:00 2001
> From: Daeseok Youn 
> Date: Wed, 19 Feb 2014 10:49:12 +0900
> Subject: [PATCH] atm: solos-pci: make solos_bh() as static
> 
> sparse says:
> 
> drivers/atm/solos-pci.c:763:6: warning:
>  symbol 'solos_bh' was not declared. Should it be static?
> 
> Signed-off-by: Daeseok Youn 
> ---
>  drivers/atm/solos-pci.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
> index e3fb496..943cf0d 100644
> --- a/drivers/atm/solos-pci.c
> +++ b/drivers/atm/solos-pci.c
> @@ -760,7 +760,7 @@ static irqreturn_t solos_irq(int irq, void *dev_id)
>   return IRQ_RETVAL(handled);
>  }
>  
> -void solos_bh(unsigned long card_arg)
> +static void solos_bh(unsigned long card_arg)
>  {
>   struct solos_card *card = (void *)card_arg;
>   uint32_t card_flags;

Acked-by: Chas Williams 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: ambassador: use NULL instead of 0 for pointer

[chas williams - CONTRACTOR]

On Wed, 19 Feb 2014 14:11:15 +0900
Daeseok Youn  wrote:

> >From 932e928d53b1e588dc17019e7f9fa7a61b8b7468 Mon Sep 17 00:00:00 2001
> From: Daeseok Youn 
> Date: Wed, 19 Feb 2014 10:35:41 +0900
> Subject: [PATCH] atm: ambassador: use NULL instead of 0 for pointer
> 
> sparse says:
> 
> drivers/atm/ambassador.c:1928:24: warning:
>  Using plain integer as NULL pointer
> 
> Signed-off-by: Daeseok Youn 
> ---
>  drivers/atm/ambassador.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
> index 62a7607..f1a9198 100644
> --- a/drivers/atm/ambassador.c
> +++ b/drivers/atm/ambassador.c
> @@ -1925,7 +1925,7 @@ static int ucode_init(loader_block *lb, amb_dev *dev)
>const struct firmware *fw;
>unsigned long start_address;
>const struct ihex_binrec *rec;
> -  const char *errmsg = 0;
> +  const char *errmsg = NULL;
>int res;
>  
>    res = request_ihex_firmware(, "atmsar11.fw", >pci_dev->dev);

Acked-by: Chas Williams 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: nicstar: use NULL instead of 0 for pointer

[chas williams - CONTRACTOR]

On Wed, 19 Feb 2014 14:12:46 +0900
Daeseok Youn  wrote:

> >From c320d2ea1ed51c88255c33a50c74fa3598ab7be6 Mon Sep 17 00:00:00 2001
> From: Daeseok Youn 
> Date: Wed, 19 Feb 2014 10:10:11 +0900
> Subject: [PATCH] atm: nicstar: use NULL instead of 0 for pointer
> 
> sparse says:
> 
> drivers/atm/nicstar.c:642:27: warning:
>  Using plain integer as NULL pointer
> drivers/atm/nicstar.c:644:27:
>  warning: Using plain integer as NULL pointer
> drivers/atm/nicstar.c:982:51:
>  warning: Using plain integer as NULL pointer
> drivers/atm/nicstar.c:996:51:
>  warning: Using plain integer as NULL pointer
> 
> Signed-off-by: Daeseok Youn 
> ---
>  drivers/atm/nicstar.c |8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
> index 9587e95..13ed54c 100644
> --- a/drivers/atm/nicstar.c
> +++ b/drivers/atm/nicstar.c
> @@ -639,9 +639,9 @@ static int ns_init_card(int i, struct pci_dev *pcidev)
>   card->hbnr.init = NUM_HB;
>   card->hbnr.max = MAX_HB;
>  
> - card->sm_handle = 0x;
> + card->sm_handle = NULL;
>   card->sm_addr = 0x;
> - card->lg_handle = 0x;
> + card->lg_handle = NULL;
>   card->lg_addr = 0x;
>  
>   card->efbie = 1;/* To prevent push_rxbufs from enabling the 
> interrupt */
> @@ -979,7 +979,7 @@ static void push_rxbufs(ns_dev * card, struct sk_buff 
> *skb)
>   addr2 = card->sm_addr;
>   handle2 = card->sm_handle;
>   card->sm_addr = 0x;
> - card->sm_handle = 0x;
> + card->sm_handle = NULL;
>   } else {/* (!sm_addr) */
>  
>   card->sm_addr = addr1;
> @@ -993,7 +993,7 @@ static void push_rxbufs(ns_dev * card, struct sk_buff 
> *skb)
>   addr2 = card->lg_addr;
>   handle2 = card->lg_handle;
>   card->lg_addr = 0x;
> -     card->lg_handle = 0x;
> + card->lg_handle = NULL;
>   } else {/* (!lg_addr) */
>  
>   card->lg_addr = addr1;

Acked-by: Chas Williams 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: nicstar: use NULL instead of 0 for pointer

[chas williams - CONTRACTOR]

On Wed, 19 Feb 2014 14:12:46 +0900
Daeseok Youn daeseok.y...@gmail.com wrote:

 From c320d2ea1ed51c88255c33a50c74fa3598ab7be6 Mon Sep 17 00:00:00 2001
 From: Daeseok Youn daeseok.y...@gmail.com
 Date: Wed, 19 Feb 2014 10:10:11 +0900
 Subject: [PATCH] atm: nicstar: use NULL instead of 0 for pointer
 
 sparse says:
 
 drivers/atm/nicstar.c:642:27: warning:
  Using plain integer as NULL pointer
 drivers/atm/nicstar.c:644:27:
  warning: Using plain integer as NULL pointer
 drivers/atm/nicstar.c:982:51:
  warning: Using plain integer as NULL pointer
 drivers/atm/nicstar.c:996:51:
  warning: Using plain integer as NULL pointer
 
 Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
 ---
  drivers/atm/nicstar.c |8 
  1 file changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
 index 9587e95..13ed54c 100644
 --- a/drivers/atm/nicstar.c
 +++ b/drivers/atm/nicstar.c
 @@ -639,9 +639,9 @@ static int ns_init_card(int i, struct pci_dev *pcidev)
   card-hbnr.init = NUM_HB;
   card-hbnr.max = MAX_HB;
  
 - card-sm_handle = 0x;
 + card-sm_handle = NULL;
   card-sm_addr = 0x;
 - card-lg_handle = 0x;
 + card-lg_handle = NULL;
   card-lg_addr = 0x;
  
   card-efbie = 1;/* To prevent push_rxbufs from enabling the 
 interrupt */
 @@ -979,7 +979,7 @@ static void push_rxbufs(ns_dev * card, struct sk_buff 
 *skb)
   addr2 = card-sm_addr;
   handle2 = card-sm_handle;
   card-sm_addr = 0x;
 - card-sm_handle = 0x;
 + card-sm_handle = NULL;
   } else {/* (!sm_addr) */
  
   card-sm_addr = addr1;
 @@ -993,7 +993,7 @@ static void push_rxbufs(ns_dev * card, struct sk_buff 
 *skb)
   addr2 = card-lg_addr;
   handle2 = card-lg_handle;
   card-lg_addr = 0x;
 - card-lg_handle = 0x;
 + card-lg_handle = NULL;
   } else {/* (!lg_addr) */
  
   card-lg_addr = addr1;

Acked-by: Chas Williams c...@cmf.nrl.navy.mil
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: solos-pci: make solos_bh() as static

[chas williams - CONTRACTOR]

On Wed, 19 Feb 2014 14:13:54 +0900
Daeseok Youn daeseok.y...@gmail.com wrote:

 From 6297aabeff748777b520cc0ee835af0a3ddc79e2 Mon Sep 17 00:00:00 2001
 From: Daeseok Youn daeseok.y...@gmail.com
 Date: Wed, 19 Feb 2014 10:49:12 +0900
 Subject: [PATCH] atm: solos-pci: make solos_bh() as static
 
 sparse says:
 
 drivers/atm/solos-pci.c:763:6: warning:
  symbol 'solos_bh' was not declared. Should it be static?
 
 Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
 ---
  drivers/atm/solos-pci.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
 index e3fb496..943cf0d 100644
 --- a/drivers/atm/solos-pci.c
 +++ b/drivers/atm/solos-pci.c
 @@ -760,7 +760,7 @@ static irqreturn_t solos_irq(int irq, void *dev_id)
   return IRQ_RETVAL(handled);
  }
  
 -void solos_bh(unsigned long card_arg)
 +static void solos_bh(unsigned long card_arg)
  {
   struct solos_card *card = (void *)card_arg;
   uint32_t card_flags;

Acked-by: Chas Williams c...@cmf.nrl.navy.mil
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: ambassador: use NULL instead of 0 for pointer

[chas williams - CONTRACTOR]

On Wed, 19 Feb 2014 14:11:15 +0900
Daeseok Youn daeseok.y...@gmail.com wrote:

 From 932e928d53b1e588dc17019e7f9fa7a61b8b7468 Mon Sep 17 00:00:00 2001
 From: Daeseok Youn daeseok.y...@gmail.com
 Date: Wed, 19 Feb 2014 10:35:41 +0900
 Subject: [PATCH] atm: ambassador: use NULL instead of 0 for pointer
 
 sparse says:
 
 drivers/atm/ambassador.c:1928:24: warning:
  Using plain integer as NULL pointer
 
 Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
 ---
  drivers/atm/ambassador.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
 index 62a7607..f1a9198 100644
 --- a/drivers/atm/ambassador.c
 +++ b/drivers/atm/ambassador.c
 @@ -1925,7 +1925,7 @@ static int ucode_init(loader_block *lb, amb_dev *dev)
const struct firmware *fw;
unsigned long start_address;
const struct ihex_binrec *rec;
 -  const char *errmsg = 0;
 +  const char *errmsg = NULL;
int res;
  
res = request_ihex_firmware(fw, atmsar11.fw, dev-pci_dev-dev);

Acked-by: Chas Williams c...@cmf.nrl.navy.mil
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: firestream: remove duplicate define

[chas williams - CONTRACTOR]

Acked-by: Chas Williams 

On Mon, 21 Oct 2013 10:12:41 +0200
Michael Opdenacker  wrote:

> This patch removes a duplicate define in drivers/atm/firestream.h
> 
> Signed-off-by: Michael Opdenacker 
> ---
>  drivers/atm/firestream.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/atm/firestream.h b/drivers/atm/firestream.h
> index 49e783e..364eded 100644
> --- a/drivers/atm/firestream.h
> +++ b/drivers/atm/firestream.h
> @@ -420,7 +420,6 @@ struct fs_transmit_config {
>  #define RC_FLAGS_BFPS_BFP27 (0xd << 17)
>  #define RC_FLAGS_BFPS_BFP47 (0xe << 17)
>  
> -#define RC_FLAGS_BFPS   (0x1 << 17)
>  #define RC_FLAGS_BFPP   (0x1 << 21)
>  #define RC_FLAGS_TEVC   (0x1 << 22)
>  #define RC_FLAGS_TEP(0x1 << 23)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm: firestream: remove duplicate define

[chas williams - CONTRACTOR]

Acked-by: Chas Williams c...@cmf.nrl.navy.mil

On Mon, 21 Oct 2013 10:12:41 +0200
Michael Opdenacker michael.opdenac...@free-electrons.com wrote:

 This patch removes a duplicate define in drivers/atm/firestream.h
 
 Signed-off-by: Michael Opdenacker michael.opdenac...@free-electrons.com
 ---
  drivers/atm/firestream.h | 1 -
  1 file changed, 1 deletion(-)
 
 diff --git a/drivers/atm/firestream.h b/drivers/atm/firestream.h
 index 49e783e..364eded 100644
 --- a/drivers/atm/firestream.h
 +++ b/drivers/atm/firestream.h
 @@ -420,7 +420,6 @@ struct fs_transmit_config {
  #define RC_FLAGS_BFPS_BFP27 (0xd  17)
  #define RC_FLAGS_BFPS_BFP47 (0xe  17)
  
 -#define RC_FLAGS_BFPS   (0x1  17)
  #define RC_FLAGS_BFPP   (0x1  21)
  #define RC_FLAGS_TEVC   (0x1  22)
  #define RC_FLAGS_TEP(0x1  23)

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 05/21] atm: he: use mdelay instead of large udelay constants

[chas williams - CONTRACTOR]

On Fri, 26 Apr 2013 09:21:59 +0100
"David Laight"  wrote:

> > ARM cannot handle udelay for more than 2 miliseconds, so we
> > should use mdelay instead for those.
> ...
> > @@ -1055,7 +1055,7 @@ static int he_start(struct atm_dev *dev)
> > he_writel(he_dev, 0x0, RESET_CNTL);
> > he_writel(he_dev, 0xff, RESET_CNTL);
> > 
> > -   udelay(16*1000);/* 16 ms */
> > +   mdelay(16); /* 16 ms */
> > status = he_readl(he_dev, RESET_CNTL);
> 
> 16ms seems a long time to spin.
> I'd have thought a sleep would be more appropriate.
> Since this looks like timing a hardware reset pulse
> it can't matter if it is somewhat longer.

Yes, I wrote this bit some time ago when I was less wise. The
programmer's guide doesn't say how long to sleep, so the value isn't
critical.  It just has to be "long enough".  An msleep() would be fine
here.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 05/21] atm: he: use mdelay instead of large udelay constants

[chas williams - CONTRACTOR]

On Fri, 26 Apr 2013 09:21:59 +0100
David Laight david.lai...@aculab.com wrote:

  ARM cannot handle udelay for more than 2 miliseconds, so we
  should use mdelay instead for those.
 ...
  @@ -1055,7 +1055,7 @@ static int he_start(struct atm_dev *dev)
  he_writel(he_dev, 0x0, RESET_CNTL);
  he_writel(he_dev, 0xff, RESET_CNTL);
  
  -   udelay(16*1000);/* 16 ms */
  +   mdelay(16); /* 16 ms */
  status = he_readl(he_dev, RESET_CNTL);
 
 16ms seems a long time to spin.
 I'd have thought a sleep would be more appropriate.
 Since this looks like timing a hardware reset pulse
 it can't matter if it is somewhat longer.

Yes, I wrote this bit some time ago when I was less wise. The
programmer's guide doesn't say how long to sleep, so the value isn't
critical.  It just has to be long enough.  An msleep() would be fine
here.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm/iphase: rename fregt_t -> ffreg_t

[chas williams - CONTRACTOR]

On Fri, 8 Feb 2013 11:19:11 +0100
Heiko Carstens  wrote:

> We have conflicting type qualifiers for "freg_t" in s390's ptrace.h and the
> iphase atm device driver, which causes the compile error below.
> Unfortunately the s390 typedef can't be renamed, since it's a user visible 
> api,
> nor can I change the include order in s390 code to avoid the conflict.
> 
> So simply rename the iphase typedef to a new name. Fixes this compile error:
> 
> In file included from drivers/atm/iphase.c:66:0:
> drivers/atm/iphase.h:639:25: error: conflicting type qualifiers for 'freg_t'
> In file included from next/arch/s390/include/asm/ptrace.h:9:0,
>  from next/arch/s390/include/asm/lowcore.h:12,
>  from next/arch/s390/include/asm/thread_info.h:30,
>  from include/linux/thread_info.h:54,
>  from include/linux/preempt.h:9,
>  from include/linux/spinlock.h:50,
>  from include/linux/seqlock.h:29,
>  from include/linux/time.h:5,
>  from include/linux/stat.h:18,
>  from include/linux/module.h:10,
>  from drivers/atm/iphase.c:43:
> next/arch/s390/include/uapi/asm/ptrace.h:197:3: note: previous declaration of 
> 'freg_t' was here
> 
> Signed-off-by: Heiko Carstens 

seems like a fine idea.

Acked-by: chas williams - CONTRACTOR 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atm/iphase: rename fregt_t - ffreg_t

[chas williams - CONTRACTOR]

On Fri, 8 Feb 2013 11:19:11 +0100
Heiko Carstens heiko.carst...@de.ibm.com wrote:

 We have conflicting type qualifiers for freg_t in s390's ptrace.h and the
 iphase atm device driver, which causes the compile error below.
 Unfortunately the s390 typedef can't be renamed, since it's a user visible 
 api,
 nor can I change the include order in s390 code to avoid the conflict.
 
 So simply rename the iphase typedef to a new name. Fixes this compile error:
 
 In file included from drivers/atm/iphase.c:66:0:
 drivers/atm/iphase.h:639:25: error: conflicting type qualifiers for 'freg_t'
 In file included from next/arch/s390/include/asm/ptrace.h:9:0,
  from next/arch/s390/include/asm/lowcore.h:12,
  from next/arch/s390/include/asm/thread_info.h:30,
  from include/linux/thread_info.h:54,
  from include/linux/preempt.h:9,
  from include/linux/spinlock.h:50,
  from include/linux/seqlock.h:29,
  from include/linux/time.h:5,
  from include/linux/stat.h:18,
  from include/linux/module.h:10,
  from drivers/atm/iphase.c:43:
 next/arch/s390/include/uapi/asm/ptrace.h:197:3: note: previous declaration of 
 'freg_t' was here
 
 Signed-off-by: Heiko Carstens heiko.carst...@de.ibm.com

seems like a fine idea.

Acked-by: chas williams - CONTRACTOR c...@cmf.nrl.navy.mil
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 02/14] atm/nicstar: don't use idr_remove_all()

[chas williams - CONTRACTOR]

On Mon, 4 Feb 2013 09:52:10 -0800
Tejun Heo  wrote:

> Subject: atm/nicstar: convert to idr_alloc()
> 
> Convert to the much saner new idr interface.  The existing code looks
> buggy to me - ID 0 is treated as no-ID but allocation specifies 0 as
> lower limit and there's no error handling after partial success.  This
> conversion keeps the bugs unchanged.
> 
> Only compile tested.
> 
> v2: id1 and id2 are now directly used for -errno return and thus
> should be signed.  Make them int instead of u32.  This was spotted
> by kbuild test robot.
> 
> Signed-off-by: Tejun Heo 
> Acked-by: Chas Williams 
> Reported-by: kbuild test robot 
> Cc: net...@vger.kernel.org
> ---
>  drivers/atm/nicstar.c |   29 +
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> --- a/drivers/atm/nicstar.c
> +++ b/drivers/atm/nicstar.c
> @@ -949,7 +949,7 @@ static void free_scq(ns_dev *card, scq_i
>  static void push_rxbufs(ns_dev * card, struct sk_buff *skb)
>  {
>   struct sk_buff *handle1, *handle2;
> - u32 id1 = 0, id2 = 0;
> + int id1 = 0, id2 = 0;
>   u32 addr1, addr2;
>   u32 stat;
>   unsigned long flags;
> @@ -1026,24 +1026,21 @@ static void push_rxbufs(ns_dev * card, s
>   card->lbfqc += 2;
>   }
>  
> - do {
> - if (!idr_pre_get(>idr, GFP_ATOMIC)) {
> - printk(KERN_ERR
> -"nicstar%d: no free memory for idr\n",
> -card->index);
> + if (id1 < 0) {

you assign id1 to 0, so this never happens i think.  i don't think the
reason to preassign id1/id2 exists anymore once the do loop is removed.

> + id1 = idr_alloc(>idr, handle1, 0, 0, GFP_ATOMIC);
> + if (id1 < 0) {
> + err = id1;

you dont need to assign err since it isn't returned.

>   goto out;
>   }
> + }
>  
> - if (!id1)
> - err = idr_get_new_above(>idr, handle1, 0, 
> );
> -
> - if (!id2 && err == 0)
> - err = idr_get_new_above(>idr, handle2, 0, 
> );
> -
> - } while (err == -EAGAIN);
> -
> - if (err)
> - goto out;
> + if (id2 < 0) {

same as above.

> + id2 = idr_alloc(>idr, handle2, 0, 0, GFP_ATOMIC);
> + if (id2 < 0) {
> + err = id2;
> + goto out;
> + }
> + }
>  
>   spin_lock_irqsave(>res_lock, flags);
>   while (CMD_BUSY(card)) ;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/62] atm/nicstar: convert to idr_alloc()

[chas williams - CONTRACTOR]

On Mon, 4 Feb 2013 09:06:24 -0800
Tejun Heo  wrote:

> Hello, Chas.
> 
> On Mon, Feb 04, 2013 at 09:04:10AM -0500, chas williams - CONTRACTOR wrote:
> > I don't quite understand your comment.  idr_pre_get() returns 0 in the
> > case of failure.
> 
> So, if you do the following,
> 
>   int id1 = 0, id2 = 0;
> 
>   if (!id1)
>   err = idr_get_new_above(>idr, handle1, 0, );
>   if (!id2 && err == 0)
>   err = idr_get_new_above(>idr, handle2, 0, );
>   if (err)
>   goto out;
>   
> You have no way of telling whether id1/2 are allocated or not.  0 is
> the special "not allocated" value but it also is a valid ID.  The
> error path should be freeing id1/2 if either of them has been
> allocated but it doesn't and can't with 0 as the non-allocated value.

yeah, i see now.  it didn't understand what idr_get_new_above() was
doing with the start id.  i assumed that it would return something
greater than the start id.  i guess it never did return the starting id
(atleast after some initial failure).

additionally, yes, cleaning up if the second allocation failed was never
done.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/62] atm/nicstar: convert to idr_alloc()

[chas williams - CONTRACTOR]

I don't quite understand your comment.  idr_pre_get() returns 0 in the
case of failure.

On Sat,  2 Feb 2013 17:20:10 -0800
Tejun Heo  wrote:

> Convert to the much saner new idr interface.  The existing code looks
> buggy to me - ID 0 is treated as no-ID but allocation specifies 0 as
> lower limit and there's no error handling after parial success.  This
> conversion keeps the bugs unchanged.
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo 
> Cc: Chas Williams 
> Cc: net...@vger.kernel.org
> ---
> This patch depends on an earlier idr changes and I think it would be
> best to route these together through -mm.  Please holler if there's
> any objection.  Thanks.
> 
>  drivers/atm/nicstar.c | 27 ---
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
> index 628787e..7fd6834 100644
> --- a/drivers/atm/nicstar.c
> +++ b/drivers/atm/nicstar.c
> @@ -1026,24 +1026,21 @@ static void push_rxbufs(ns_dev * card, struct sk_buff 
> *skb)
>   card->lbfqc += 2;
>   }
>  
> - do {
> - if (!idr_pre_get(>idr, GFP_ATOMIC)) {
> - printk(KERN_ERR
> -"nicstar%d: no free memory for idr\n",
> -card->index);
> + if (!id1) {
> + id1 = idr_alloc(>idr, handle1, 0, 0, GFP_ATOMIC);
> + if (id1 < 0) {
> + err = id1;
>   goto out;
>   }
> + }
>  
> - if (!id1)
> - err = idr_get_new_above(>idr, handle1, 0, 
> );
> -
> - if (!id2 && err == 0)
> - err = idr_get_new_above(>idr, handle2, 0, 
> );
> -
> - } while (err == -EAGAIN);
> -
> - if (err)
> - goto out;
> + if (!id2) {
> + id2 = idr_alloc(>idr, handle2, 0, 0, GFP_ATOMIC);
> + if (id2 < 0) {
> + err = id2;
> + goto out;
> + }
> + }
>  
>   spin_lock_irqsave(>res_lock, flags);
>   while (CMD_BUSY(card)) ;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/62] atm/nicstar: convert to idr_alloc()

[chas williams - CONTRACTOR]

I don't quite understand your comment.  idr_pre_get() returns 0 in the
case of failure.

On Sat,  2 Feb 2013 17:20:10 -0800
Tejun Heo t...@kernel.org wrote:

 Convert to the much saner new idr interface.  The existing code looks
 buggy to me - ID 0 is treated as no-ID but allocation specifies 0 as
 lower limit and there's no error handling after parial success.  This
 conversion keeps the bugs unchanged.
 
 Only compile tested.
 
 Signed-off-by: Tejun Heo t...@kernel.org
 Cc: Chas Williams c...@cmf.nrl.navy.mil
 Cc: net...@vger.kernel.org
 ---
 This patch depends on an earlier idr changes and I think it would be
 best to route these together through -mm.  Please holler if there's
 any objection.  Thanks.
 
  drivers/atm/nicstar.c | 27 ---
  1 file changed, 12 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
 index 628787e..7fd6834 100644
 --- a/drivers/atm/nicstar.c
 +++ b/drivers/atm/nicstar.c
 @@ -1026,24 +1026,21 @@ static void push_rxbufs(ns_dev * card, struct sk_buff 
 *skb)
   card-lbfqc += 2;
   }
  
 - do {
 - if (!idr_pre_get(card-idr, GFP_ATOMIC)) {
 - printk(KERN_ERR
 -nicstar%d: no free memory for idr\n,
 -card-index);
 + if (!id1) {
 + id1 = idr_alloc(card-idr, handle1, 0, 0, GFP_ATOMIC);
 + if (id1  0) {
 + err = id1;
   goto out;
   }
 + }
  
 - if (!id1)
 - err = idr_get_new_above(card-idr, handle1, 0, 
 id1);
 -
 - if (!id2  err == 0)
 - err = idr_get_new_above(card-idr, handle2, 0, 
 id2);
 -
 - } while (err == -EAGAIN);
 -
 - if (err)
 - goto out;
 + if (!id2) {
 + id2 = idr_alloc(card-idr, handle2, 0, 0, GFP_ATOMIC);
 + if (id2  0) {
 + err = id2;
 + goto out;
 + }
 + }
  
   spin_lock_irqsave(card-res_lock, flags);
   while (CMD_BUSY(card)) ;

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/62] atm/nicstar: convert to idr_alloc()

[chas williams - CONTRACTOR]

On Mon, 4 Feb 2013 09:06:24 -0800
Tejun Heo t...@kernel.org wrote:

 Hello, Chas.
 
 On Mon, Feb 04, 2013 at 09:04:10AM -0500, chas williams - CONTRACTOR wrote:
  I don't quite understand your comment.  idr_pre_get() returns 0 in the
  case of failure.
 
 So, if you do the following,
 
   int id1 = 0, id2 = 0;
 
   if (!id1)
   err = idr_get_new_above(card-idr, handle1, 0, id1);
   if (!id2  err == 0)
   err = idr_get_new_above(card-idr, handle2, 0, id2);
   if (err)
   goto out;
   
 You have no way of telling whether id1/2 are allocated or not.  0 is
 the special not allocated value but it also is a valid ID.  The
 error path should be freeing id1/2 if either of them has been
 allocated but it doesn't and can't with 0 as the non-allocated value.

yeah, i see now.  it didn't understand what idr_get_new_above() was
doing with the start id.  i assumed that it would return something
greater than the start id.  i guess it never did return the starting id
(atleast after some initial failure).

additionally, yes, cleaning up if the second allocation failed was never
done.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 02/14] atm/nicstar: don't use idr_remove_all()

[chas williams - CONTRACTOR]

On Mon, 4 Feb 2013 09:52:10 -0800
Tejun Heo t...@kernel.org wrote:

 Subject: atm/nicstar: convert to idr_alloc()
 
 Convert to the much saner new idr interface.  The existing code looks
 buggy to me - ID 0 is treated as no-ID but allocation specifies 0 as
 lower limit and there's no error handling after partial success.  This
 conversion keeps the bugs unchanged.
 
 Only compile tested.
 
 v2: id1 and id2 are now directly used for -errno return and thus
 should be signed.  Make them int instead of u32.  This was spotted
 by kbuild test robot.
 
 Signed-off-by: Tejun Heo t...@kernel.org
 Acked-by: Chas Williams c...@cmf.nrl.navy.mil
 Reported-by: kbuild test robot fengguang...@intel.com
 Cc: net...@vger.kernel.org
 ---
  drivers/atm/nicstar.c |   29 +
  1 file changed, 13 insertions(+), 16 deletions(-)
 
 --- a/drivers/atm/nicstar.c
 +++ b/drivers/atm/nicstar.c
 @@ -949,7 +949,7 @@ static void free_scq(ns_dev *card, scq_i
  static void push_rxbufs(ns_dev * card, struct sk_buff *skb)
  {
   struct sk_buff *handle1, *handle2;
 - u32 id1 = 0, id2 = 0;
 + int id1 = 0, id2 = 0;
   u32 addr1, addr2;
   u32 stat;
   unsigned long flags;
 @@ -1026,24 +1026,21 @@ static void push_rxbufs(ns_dev * card, s
   card-lbfqc += 2;
   }
  
 - do {
 - if (!idr_pre_get(card-idr, GFP_ATOMIC)) {
 - printk(KERN_ERR
 -nicstar%d: no free memory for idr\n,
 -card-index);
 + if (id1  0) {

you assign id1 to 0, so this never happens i think.  i don't think the
reason to preassign id1/id2 exists anymore once the do loop is removed.

 + id1 = idr_alloc(card-idr, handle1, 0, 0, GFP_ATOMIC);
 + if (id1  0) {
 + err = id1;

you dont need to assign err since it isn't returned.

   goto out;
   }
 + }
  
 - if (!id1)
 - err = idr_get_new_above(card-idr, handle1, 0, 
 id1);
 -
 - if (!id2  err == 0)
 - err = idr_get_new_above(card-idr, handle2, 0, 
 id2);
 -
 - } while (err == -EAGAIN);
 -
 - if (err)
 - goto out;
 + if (id2  0) {

same as above.

 + id2 = idr_alloc(card-idr, handle2, 0, 0, GFP_ATOMIC);
 + if (id2  0) {
 + err = id2;
 + goto out;
 + }
 + }
  
   spin_lock_irqsave(card-res_lock, flags);
   while (CMD_BUSY(card)) ;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 10/14] atm: Removed redundant check on unsigned variable

[chas williams - CONTRACTOR]

Acked-by: chas williams - CONTRACTOR 

On Fri, 28 Dec 2012 10:46:36 +0530
Tushar Behera  wrote:

> Ping.
> 
> On 11/16/2012 12:20 PM, Tushar Behera wrote:
> > No need to check whether unsigned variable is less than 0.
> > 
> > CC: Chas Williams 
> > CC: linux-atm-gene...@lists.sourceforge.net
> > CC: net...@vger.kernel.org
> > Signed-off-by: Tushar Behera 
> > ---
> >  drivers/atm/fore200e.c |2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/atm/fore200e.c b/drivers/atm/fore200e.c
> > index 361f5ae..fdd3fe7 100644
> > --- a/drivers/atm/fore200e.c
> > +++ b/drivers/atm/fore200e.c
> > @@ -972,7 +972,7 @@ int bsq_audit(int where, struct host_bsq* bsq, int 
> > scheme, int magn)
> >where, scheme, magn, buffer->index, buffer->scheme);
> > }
> >  
> > -   if ((buffer->index < 0) || (buffer->index >= fore200e_rx_buf_nbr[ 
> > scheme ][ magn ])) {
> > +   if (buffer->index >= fore200e_rx_buf_nbr[ scheme ][ magn ]) {
> > printk(FORE200E "bsq_audit(%d): queue %d.%d, out of range buffer 
> > index = %ld !\n",
> >where, scheme, magn, buffer->index);
> > }
> > 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 10/14] atm: Removed redundant check on unsigned variable

[chas williams - CONTRACTOR]

Acked-by: chas williams - CONTRACTOR c...@cmf.nrl.navy.mil

On Fri, 28 Dec 2012 10:46:36 +0530
Tushar Behera tushar.beh...@linaro.org wrote:

 Ping.
 
 On 11/16/2012 12:20 PM, Tushar Behera wrote:
  No need to check whether unsigned variable is less than 0.
  
  CC: Chas Williams c...@cmf.nrl.navy.mil
  CC: linux-atm-gene...@lists.sourceforge.net
  CC: net...@vger.kernel.org
  Signed-off-by: Tushar Behera tushar.beh...@linaro.org
  ---
   drivers/atm/fore200e.c |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
  
  diff --git a/drivers/atm/fore200e.c b/drivers/atm/fore200e.c
  index 361f5ae..fdd3fe7 100644
  --- a/drivers/atm/fore200e.c
  +++ b/drivers/atm/fore200e.c
  @@ -972,7 +972,7 @@ int bsq_audit(int where, struct host_bsq* bsq, int 
  scheme, int magn)
 where, scheme, magn, buffer-index, buffer-scheme);
  }
   
  -   if ((buffer-index  0) || (buffer-index = fore200e_rx_buf_nbr[ 
  scheme ][ magn ])) {
  +   if (buffer-index = fore200e_rx_buf_nbr[ scheme ][ magn ]) {
  printk(FORE200E bsq_audit(%d): queue %d.%d, out of range buffer 
  index = %ld !\n,
 where, scheme, magn, buffer-index);
  }
  
 
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/3] pppoatm: protect against freeing of vcc

[chas williams - CONTRACTOR]

On Fri, 30 Nov 2012 16:23:46 +
David Woodhouse  wrote:

> On Fri, 2012-11-30 at 12:10 +, David Woodhouse wrote:
> > In that case I think we're fine. I'll just do the same thing in
> > br2684_push(), fix up the comment you just corrected, and we're all
> > good.
> 
> OK, here's an update to me my patch 8/17 'br2684: don't send frames on
> not-ready vcc'. It takes the socket lock and does fairly much the same
> thing as your pppoatm version. It returns NETDEV_TX_BUSY and stops the
> queue if the socket is locked, and it gets woken from the ->release_cb
> callback.
> 
> I've dropped your Acked-By: since it's mostly new, but feel free to give
> me a fresh one. With this I think we're done.
> 
> Unless Chas has any objections, I'll ask Dave to pull it...

no objections.  i think this deals with my concerns.  as for splitting
the close functions, from one of your previous messages:


>Really, what we're saying is that *one* of the driver or protocol close
>functions needs to be split, and we need to do DPD or PDP. Since the
>device driver *can* abort/flush the TX queue and also any pending RX
>being handled by a tasklet, I think it makes most sense to keep it in
>the middle, with the protocol being handled first and last... which is
>the current order, as long as we consider setting ATM_VF_CLOSE to be the
>first part.

i believe this is essentially already done with the release_cb()
implementation right?  that is splitting the protocol detach/shutdown
into two parts.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/3] pppoatm: protect against freeing of vcc

[chas williams - CONTRACTOR]

On Fri, 30 Nov 2012 16:23:46 +
David Woodhouse dw...@infradead.org wrote:

 On Fri, 2012-11-30 at 12:10 +, David Woodhouse wrote:
  In that case I think we're fine. I'll just do the same thing in
  br2684_push(), fix up the comment you just corrected, and we're all
  good.
 
 OK, here's an update to me my patch 8/17 'br2684: don't send frames on
 not-ready vcc'. It takes the socket lock and does fairly much the same
 thing as your pppoatm version. It returns NETDEV_TX_BUSY and stops the
 queue if the socket is locked, and it gets woken from the -release_cb
 callback.
 
 I've dropped your Acked-By: since it's mostly new, but feel free to give
 me a fresh one. With this I think we're done.
 
 Unless Chas has any objections, I'll ask Dave to pull it...

no objections.  i think this deals with my concerns.  as for splitting
the close functions, from one of your previous messages:


Really, what we're saying is that *one* of the driver or protocol close
functions needs to be split, and we need to do DPD or PDP. Since the
device driver *can* abort/flush the TX queue and also any pending RX
being handled by a tasklet, I think it makes most sense to keep it in
the middle, with the protocol being handled first and last... which is
the current order, as long as we consider setting ATM_VF_CLOSE to be the
first part.

i believe this is essentially already done with the release_cb()
implementation right?  that is splitting the protocol detach/shutdown
into two parts.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/