Re: [PATCH 1/1 net-next] net/atm/signaling.c: replace current->state by __set_current_state()
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(&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
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
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(&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);
Re: [PATCH v2 1/1] atm: remove deprecated use of pci api
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 = &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
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
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] drivers:atm Remove two FIXMES in the function, top_off_fp for the file, firestream.c
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: FIXME in solos-pci.c
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(&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
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, &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(
Re: [setsockopt] WARNING: CPU: 0 PID: 1444 at kernel/sched/core.c:7088 __might_sleep+0x51/0x16f()
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, &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 3/5] drivers/atm/atmtcp.c: fix error return code
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(&vcc_sklist_lock);
> if (!out_vcc) {
> + result = -EUNATCH;
> atomic_inc(&vcc->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
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(&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
--
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
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(&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 02/19] drivers: atm: fix %d confusingly prefixed with 0x in format strings
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: solos-pci.c: Fix me
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(&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: solos-pci.c: Fix me
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(&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: [PATCH] atm: solos-pci: make solos_bh() as static
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
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(&fw, "atmsar11.fw", &dev->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
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: firestream: remove duplicate define
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 05/21] atm: he: use mdelay instead of large udelay constants
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] atm/iphase: rename fregt_t -> ffreg_t
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 v2 02/14] atm/nicstar: don't use idr_remove_all()
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(&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 09/62] atm/nicstar: convert to idr_alloc()
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(&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 09/62] atm/nicstar: convert to idr_alloc()
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(&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 10/14] atm: Removed redundant check on unsigned variable
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 v2 3/3] pppoatm: protect against freeing of vcc
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
In message <1354227428.21562.230.ca...@shinybook.infradead.org>,David Woodhouse writes: >At this point, I think we're better off as we are (with Krzysztof's >patch 1/7 dropped, and leaving vcc->dev->ops->close() being called >before vcc->push(NULL). We've fairly much solved the issues with that >arrangement, by checking ATM_VF_READY in the protocols' ->push() >functions. it isnt clear to me that fixes the race entirely either. vcc_destroy_socket() and any of the push()/sends()'s are not serialized. while you may clear the ATM_VF_READY flag, you might not clear it soon enough for any particular push() that is already running. so it still seems like you are racing close() against push() at this point. the window is greatly reduced, but it still exists. -- 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
On Thu, 29 Nov 2012 18:11:48 + David Woodhouse wrote: > We do see the 'packet received for unknown VCC' complaint, after we > reboot the host without resetting the card. And as shown in the commit I > just referenced, we rely on the !ATM_VF_READY check in order to prevent > a use-after-free when the tasklet is sending packets up a VCC that's > just been closed. well that behavior is just crap. why is it delivering cells for a vpi/vci pair that is not open? regardless, i pointed out the behavior of find_vcc() just in case you need to do some clean up on data that is coming back while you are attempting to finish operations during your driver's close() but this cleanup might not be happening since you arent able to get a reference to the vcc so you can pop() the data or whatever you might need to do. > > again part of this is poor synchronization. the detach protocol (i.e. > > push of a NULL skb) should be flushing any pending transmits and > > shutting down whatever in the protocol is doing any sending and > > receiving. however, i release this might be difficult to do since the > > detach protocol is invoked in such a strange way. > > You mean that (e.g.) pppoatm_push(vcc, NULL) should be waiting for any > pending TX skb (that has already been passed off to the driver) to > *complete*? How would it even do that? i think the order of the vcc_destroy_socket() operations is a bit wrong. it should call detach protocol (i.e. push a NULL). this should cause the attached protocol to stop any future sends and receives, and it CAN sleep in this context (and only this context -- generally sending cannot sleep which is why this might seem confusing) to do whatever it needs to do to wait for the attached protocol to clean up queues, flush data etc. then the driver close() should be called. this takes care of cleaning up any pending tx or rx that is in the hardware. and of course, close() can sleep since it will be in a interrutible context. the pop() might be screwed up here though. you might have skb's in the driver that should be pop()'ed with the formerly attached protocol. you could wait for pending tx's sent to the driver. you know that your attached protocol's pop() will be called. keep count of the outstanding transmit skb's. you cant do this though if you close() the vcc before you detach the protocol. -- 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
On Thu, 29 Nov 2012 16:24:29 + David Woodhouse wrote: > On Thu, 2012-11-29 at 10:59 -0500, chas williams - CONTRACTOR wrote: > > the part that bothers me (and i dont have the programmer's guide for > > the solos hardware) is that you are watching for the PKT_PCLOSE to be > > sent to the card. shouldnt you be watching for the PKT_PCLOSE to be > > returned from the card (assuming it does such a thing) so that you can > > be assured that the tx/rx for this vpi/vci pair has been "stopped"? > > Define "stopped". > > For the RX case... the other end may *always* take it upon itself to > send us a packet marked with arbitrary VCI/VPI, right? There's no > connection setup for it "on the wire", in the case of PVC? most atm hardware that i am familiar with, wont deliver vpi/vci data unless you are actually trying to receive it. however, this hardware is generally doing its reassembly in hardware and delivering aal5 pdu's and needs to manage its memory resources carefully. you might be trying to reassemble 1000 pdu's from different vpi/vci's. > So bearing that in mind: from the moment ATM_VF_READY gets cleared, as > far as the ATM core is concerned, we will no longer receive packets on > the given VCC. If we receive any, we'll just complain about receiving > packets for an unknown VCI/VPI. > > For the TX case ... yes, we need to be sure we aren't continuing to send > packets after our close() routine completes. We *used* to, but the > resulting ->pop() calls were causing problems, and that's why we're > looking at this code path closer. The currently proposed patches (except > one suggestion from Krzyztof that we both shouted down) would fix that. again part of this is poor synchronization. the detach protocol (i.e. push of a NULL skb) should be flushing any pending transmits and shutting down whatever in the protocol is doing any sending and receiving. however, i release this might be difficult to do since the detach protocol is invoked in such a strange way. -- 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
On Thu, 29 Nov 2012 15:59:08 + David Woodhouse wrote: > On Thu, 2012-11-29 at 10:37 -0500, chas williams - CONTRACTOR wrote: > > you shouldnt clear ATM_VF_ADDR until the vpi/vci is actually closed and > > ready for reuse. at this point, it isnt. > > So I should always wait for the completion of my PKT_CLOSE and only > clear ATM_VF_ADDR when it's actually done? > > But can you define 'ready for reuse'? From the moment I clear > ATM_VF_ADDR, another CPU may enter my popen() function to set up another > VCC with the same parameters, and everything should work fine. The > PKT_POPEN will end up on the queue *after* my PKT_PCLOSE for the old > VCC. Any received packets will be dropped until the new VCC gets > ATM_VF_READY set (by the popen function). > > What's the actual failure mode, caused by me clearing ATM_VF_ADDR "too > early"? there may not be one (due to serialization from other parts of the atm stack) but you "shouldn't" clear ATM_VF_ADDR until the vpi/vci pair is ready for reuse. by reuse, i mean that any previous rx/tx data in the vpi/vci segmentation hardware has been removed/cleared. > > ATM_VF_READY should already be clear at this point but you should set > > it before you queue your PKT_CLOSE. > > I should *set* it? Do you mean clear it? Yes, I see it's cleared by sorry, i did mean clear it. > vcc_destroy_socket()... but all the other ATM drivers also seem to clear > it for themselves, and that would appear to be harmless. yeah, like i said, it is spuriously cleared in the drivers and should probably just be moved to under the control of the next layer up completely. drivers/atm should just handle the hardware side, not the software side. > > checking for ATM_VF_READY in find_vcc() is probably going to give you > > grief as well since ATM_VF_READY isnt entirely under your control. > > That's fine. If *anyone* has cleared ATM_VF_READY, I stop sending > packets up it. Or, more to the point, I stop using the damn thing at > all. See commit 1f6ea6e511e5ec730d8e88651da1b7b6e8fd1333. > > > you need to be able to find the vcc until after pclose() is finished since > > your tasklet might have a few packets it is still processing? > > The whole point of that check is that the tasklet *won't* be able to > find it any more, and it'll just discard incoming packets for the > obsolescent VCC. that's fine as long as you understand this. in the case of the he, i needed to be able to find the vcc until close() is finished so that i can wakeup the sleeper in the close() routine that is waiting for the reassembly queue to be cleared/reset. also, i still needed to find the vcc for the tx side during close() since i still might need to pop() skb's that are being sent during the close() while i am still trying to get the hardware to shutdown the transmit dma engine. -- 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
On Thu, 29 Nov 2012 15:47:57 +
David Woodhouse wrote:
> @@ -1020,12 +1048,15 @@ static uint32_t fpga_tx(struct solos_card *card)
> if (vcc) {
> atomic_inc(&vcc->stats->tx);
> solos_pop(vcc, oldskb);
> - } else {
> - struct pkt_hdr *header = (void *)oldskb->data;
> - if (le16_to_cpu(header->type) == PKT_PCLOSE)
> - complete(&SKB_CB(oldskb)->c);
> +
> + /*
> + * If it's a TX skb on a closed VCC, pclose()
> + * may be waiting for it...
> + */
> + if (!test_bit(ATM_VF_READY, &vcc->flags))
> + wake_up(&card->param_wq);
> + } else
> dev_kfree_skb_irq(oldskb);
> - }
the part that bothers me (and i dont have the programmer's guide for
the solos hardware) is that you are watching for the PKT_PCLOSE to be
sent to the card. shouldnt you be watching for the PKT_PCLOSE to be
returned from the card (assuming it does such a thing) so that you can
be assured that the tx/rx for this vpi/vci pair has been "stopped"?
--
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
On Wed, 28 Nov 2012 22:18:35 +
David Woodhouse wrote:
> diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
> index 9851093..3720670 100644
> --- a/drivers/atm/solos-pci.c
> +++ b/drivers/atm/solos-pci.c
> @@ -92,6 +92,7 @@ struct pkt_hdr {
> };
>
> struct solos_skb_cb {
> + struct completion c;
> struct atm_vcc *vcc;
> uint32_t dma_addr;
> };
> @@ -881,11 +882,18 @@ static void pclose(struct atm_vcc *vcc)
> header->vci = cpu_to_le16(vcc->vci);
> header->type = cpu_to_le16(PKT_PCLOSE);
>
> + init_completion(&SKB_CB(skb)->c);
> +
> fpga_queue(card, SOLOS_CHAN(vcc->dev), skb, NULL);
>
> clear_bit(ATM_VF_ADDR, &vcc->flags);
> clear_bit(ATM_VF_READY, &vcc->flags);
you shouldnt clear ATM_VF_ADDR until the vpi/vci is actually closed and
ready for reuse. at this point, it isnt. ATM_VF_READY should already
be clear at this point but you should set it before you queue your
PKT_CLOSE. these flags probably should be handled outside the drivers
since the context for them is pretty clear. just another patch i never
got around to writing...
checking for ATM_VF_READY in find_vcc() is probably going to give you
grief as well since ATM_VF_READY isnt entirely under your control. you
need to be able to find the vcc until after pclose() is finished since
your tasklet might have a few packets it is still processing?
--
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] solos-pci: don't call vcc->pop() after pclose()
On Thu, 29 Nov 2012 13:43:44 +0100 Krzysztof Mazur wrote: > Removing packets from tx_queue is not needed. We can transmit packets > also after close. We just can't call vcc->pop() after close, > so we can just set SKB_CB(skb)->vcc of such packets to NULL so fpga_tx() > won't call vcc->pop(). i dont think you can transmit packets after close(). you can transmit packets during close() though. if you transmit after close that means that you are using the vpi/vci pair that the atm stack thinks is no longer in use. additionally after close(), the hardware should be in a state such that you cannot transmit or receive on the vpi/vci that has been closed. close() needs to make sure that any pending tx packets are sent or otherwise disposed of (like turning off the transmit segmentation engine, clearing the packets, or whatever). any partially reassembled pdu's also need to be cleared as well. -- 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
On Thu, 29 Nov 2012 11:57:15 +0100 Krzysztof Mazur wrote: > or if we really want to call vcc->pop() for such skbs: you need to call ->pop() to cleaning up the wmem_alloc accounting. otherwise you will get complaints from the atm stack later about freeing a vcc that had outstanding data. -- 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: introduce vcc_pop()
On Wed, 28 Nov 2012 22:45:34 +0100 Krzysztof Mazur wrote: > On Wed, Nov 28, 2012 at 04:20:01PM -0500, chas williams - CONTRACTOR wrote: > > i dont like the vcc->pop() implementation and at one point i had the > > crazy idea of using skb->destructors to handle it. however, i think it > > would be necessary to clone the skb's so any existing destructor is > > preserved. > > With this patch we will kill vcc->pop() in drivers and in future > we can do that without changes in drivers. ok > > > > > +#define vcc_pop(vcc, skb) vcc_pop_any(vcc, skb) > > > +#define vcc_pop_irq(vcc, skb) vcc_pop_any(vcc, skb) > > > > don't define these if you dont plan on using them anway. > > I removed them. I also added check if vcc is NULL, as David Woodhouse > suggested, some drivers use that. it should probably be if (likely(vcc) && likely(vcc->pop)) since it will almost always be the case. -- 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
On Wed, 28 Nov 2012 21:18:37 +0100 Krzysztof Mazur wrote: > On Tue, Nov 27, 2012 at 07:28:43PM +0100, Krzysztof Mazur wrote: > > I think that we should add atm_pop() function that does that and fix all > > drivers. > > > > I'm sending a patch that implements that idea. > > Currently we need two arguments vcc and skb. However, we have reserved > ATM_SKB(skb)->vcc in skb control block for keeping vcc > and we can create single argument version vcc_pop(skb). In that case > we need to move: > > ATM_SKB(skb)->vcc = vcc; > > from ATM drivers to functions that call atmdev_ops->send(). i dont like the vcc->pop() implementation and at one point i had the crazy idea of using skb->destructors to handle it. however, i think it would be necessary to clone the skb's so any existing destructor is preserved. > +#define vcc_pop(vcc, skb) vcc_pop_any(vcc, skb) > +#define vcc_pop_irq(vcc, skb) vcc_pop_any(vcc, skb) don't define these if you dont plan on using them anway. -- 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
On Wed, 28 Nov 2012 10:24:28 + David Woodhouse wrote: > On Wed, 2012-11-28 at 11:04 +0100, Krzysztof Mazur wrote: > > > > The ->close() routine can just abort any pending rx/tx and just wait > > for completion of currently running rx/tx code. That shouldn't take > > long. > > If it's been submitted to the hardware for DMA, it can't do that very > easily. > > And if I can't be bothered to write code to go through the entire damn > queue and inspect every packet to see if it's a data packet and check > the VCI/VPI and try to steal it, it can't be done for the software queue > either :) > > The queue ought to be short; if it isn't, then we already screwed up. > The close therefore should be quick, and it *doesn't* have to be > instant. > > If someone wants to return immediately, there's always > vcc_release_async()... > i dont think that would be quite the right way to do it. vcc_release_async() just mark's the vcc for deletion--you still need to go through and close it eventually. however, nothing would prevent you from writing a close routine that could just reschedule something periodically to check to see if the hardware finally finished closing the vcc and can be reused. the part that needs fixed for this would be marking the vcc for reuse. you would need to keep the vpi.vci marked as busy so that someone else doesnt try to reuse it while it is closing. right now, vcc_destroy_socket() always removes the vcc from the vcc list -- regardless of whether or not close fully succeeded. -- 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
On Tue, 27 Nov 2012 18:02:29 + David Woodhouse wrote: > In solos-pci at least, the ops->close() function doesn't flush all > pending skbs for this vcc before returning. So can be a tasklet > somewhere which has loaded the address of the vcc->pop function from one > of them, and is going to call it in some unspecified amount of time. > > Should we make the device's ->close function wait for all TX and RX skbs > for this vcc to complete? the driver's close routine should wait for any of the pending tx and rx to complete. take a look at the he.c in driver/atm -- 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 8/7] pppoatm: fix missing wakeup in pppoatm_send()
On Tue, 27 Nov 2012 13:27:47 + David Woodhouse wrote: > > i really would prefer not to use a strange name since it might confuse > > larger group of people who are more familiar with the traditional meaning > > of this function. vcc_release() isnt exported so we could rename it if > > things get too confusing. > > > > i have to look at this a bit more but we might be able to use release_cb > > to get rid of the null push to detach the underlying protocol. that would > > be somewhat nice. > > In the meantime, should I resend this patch with the name 'release_cb' > instead of 'unlock_cb'? I'll just put a comment in to make sure it isn't > confused with vcc_release(), and if we need to change vcc_release() > later we can. > yes, but dont call it 8/7 since that doesnt make sense. -- 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 8/7] pppoatm: fix missing wakeup in pppoatm_send()
In message <1352667081.9449.135.ca...@shinybook.infradead.org>,David Woodhouse writes: >Acked-by: David Woodhouse for your new >version of patch #6 (returning DROP_PACKET for !VF_READY), and your >followup to my patch #8, adding the 'need_wakeup' flag. Which we might >as well merge into (the pppoatm part of) my patch. > >Chas, are you happy with the generic ATM part of that? And the >nomenclature? I didn't want to call it 'release_cb' like the core socket >code does, because we use 'release' to mean something different in ATM. >So I called it 'unlock_cb' instead... i really would prefer not to use a strange name since it might confuse larger group of people who are more familiar with the traditional meaning of this function. vcc_release() isnt exported so we could rename it if things get too confusing. i have to look at this a bit more but we might be able to use release_cb to get rid of the null push to detach the underlying protocol. that would be somewhat nice. -- 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 8/7] pppoatm: fix missing wakeup in pppoatm_send()
In message <2012110437.ga25...@shrek.podlesie.net>,Krzysztof Mazur writes: >Any race with testing vcc flags is probably not really important >because: > - vcc_release_async() does not take any lock so this check > will be always racy so we are probably allowed to send > new frames until vcc is really closed, this locking/synchronization is handled in the atm device drivers. the send and close routines are responsbile for synchronization (yes, i believe that leads to duplicated code but that is the way it currently is) -- 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 2/7] atm: add owner of push() callback to atmvcc
On Tue, 6 Nov 2012 23:16:57 +0100 Krzysztof Mazur wrote: > The atm is using atmvcc->push(vcc, NULL) callback to notify protocol > that vcc will be closed and protocol must detach from it. This callback > is usually used by protocol to decrement module usage count by module_put(), > but it leaves small window then module is still used after module_put(). > > Now the owner of push() callback is kept in atmvcc and > module_put(atmvcc->owner) is called after the protocol is detached from vcc. > > Signed-off-by: Krzysztof Mazur 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 v2 2/3] pppoatm: fix race condition with destroying of vcc
On Wed, 31 Oct 2012 23:04:35 +0100 Krzysztof Mazur wrote: > There are also some minor potential issues in pppoatm driver: > > - locking issues, but now only between pppoatm_send() and > vcc_sendmsg() and maybe some other functions, these have been around for a while. i agree that something should be done about it. just not sure what should be synchronizing this mess. > - missing check for SS_CONNECTED in pppoatm_ioctl, in practice you will never run into this because a pvc is immediately put into SS_CONNECTED mode (right before the userspace open() returns). however, should it check? yes. i dont see anything preventing you from running ppp on svc's. -- 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 2/3] pppoatm: fix race condition with destroying of vcc
On Wed, 31 Oct 2012 10:41:47 +0100 Krzysztof Mazur wrote: > On Tue, Oct 30, 2012 at 07:20:01PM +0100, Krzysztof Mazur wrote: > > Yes, this problem can be probably fixed by reversing close and push > > and adding some synchronization to pppoatm_unassign_vcc(), but I think > > we need that locking anyway, for instance for synchronization for > > checking and incrementing sk->sk_wmem_alloc, between pppoatm_send() > > and vcc_sendmsg(). > > > > I think that the same problem exists in other drivers (net/atm/br2684.c, > net/atm/clip.c, maybe other). > > Reversing order of close() and push(vcc, NULL) operations seems to > be a good idea, but synchronization with push(vcc, NULL) > and function that calls vcc->send() must be added to all drivers. this was the scheme that was (and is) currently in place. detaching a protocol from the atm layer never had a separate function, so it was decided at some point to just push a NULL skb as a signal to the next layer that i needed to cleanly shutdown and detach. the push(vcc, NULL) always happens in a sleepable context, so waiting for whatever attached protocol scheduler to finish up is not a problem. after the pushing of the skb NULL, the attached protocol should not send or recv any data on that vcc. reversing the order of the push and close certainly seems like the right thing to do. i would like to see if it would fix your problem. making the minimal change to get something working would be preferred before adding additional complexity. i am just surprised we havent seen this bug before. > I think it's better to just use ATM socket lock - lock_sock(sk_atm(vcc)), > it will fix also problems with synchronization with vcc_sendmsg() > and possibly other functions (ioctl?). > > I think that we should add a wrapper to vcc->send(), based on > fixed pppoatm_send(), that performs required checks and takes the ATM socket > lock. > > But I think we should reverse those operations anyway, because some > drivers may use other locks, not ATM socket lock, for proper > synchronization. i dont think this is a bad idea. vcc_release_async() could happen (this would be a bit unusual for a pvc but removing the usbatm device would do this) and there is no point in sending on a vcc that is closing. -- 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 2/3] pppoatm: fix race condition with destroying of vcc
In message <1350926091-12642-2-git-send-email-krzys...@podlesie.net>,Krzysztof Mazur writes: >The pppoatm_send() calls vcc->send() and now also checks for >some vcc flags that indicate destroyed vcc without proper locking. ... >The vcc_sendmsg() uses lock_sock(sk). This lock is used by >vcc_release(), so vcc_destroy_socket() will not be called between >check and during ->send(). The vcc_release_async() sets ATM_VF_CLOSE, >but it should be safe to call ->send() after it, because >vcc->dev->ops->close() is not called. as i recall from way back, this shouldnt be necessary. closing a vcc for an attached protocol isnt supposed to require addtional locking or synchronization. vcc_release() locks the socket and vcc_destroy_socket() calls the device's vcc close routine and pushes a NULL skb to the attached protocol. this NULL push is supposed to let the attached protocol that no more sends and recvs can be handled. that said, the order for the device vcc close and push does seem reversed. since i imagine there could be a pending pppoatm_send() during this interval. the push of the NULL skb is allowed to wait for the subprotocol to finish its cleanup/shutdown. -- 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: Build failure when installing atm ambassador firmware
On Tue, 31 Jul 2012 21:59:37 -0400 Shea Levy wrote: > Hello, > > When building with > MODLIB=/nix/store/ghx6s9hnk9irim7c7f63zrxqiv6xjh3w-linux-3.5/lib/modules/3.5.0 > > and > ="/nix/store/ghx6s9hnk9irim7c7f63zrxqiv6xjh3w-linux-3.5/lib/firmware", > building Linux 3.5 with CONFIG_ATM_AMBASSADOR=m fails with: > > "make[2]: *** No rule to make target > `"/nix/store/ghx6s9hnk9irim7c7f63zrxqiv6xjh3w-linux-3.5/lib/firmware"/./', > needed by > `"/nix/store/ghx6s9hnk9irim7c7f63zrxqiv6xjh3w-linux-3.5/lib/firmware"/atmsar11.fw'. > > Stop." i think it might be trying to tell you that the target directory $INSTALL_FW_PATH doesn't exist and is a prerequisite to actually perform the action. it complains about atmsar11.fw first because it just happens to be the first firmware that needs to be installed. /scratch/chas/net-next relax.53% ls -ld /tmp/funk ls: cannot access /tmp/funk: No such file or directory /scratch/chas/net-next relax.54% make INSTALL_FW_PATH="/tmp/funk" firmware_install make[1]: *** No rule to make target `/tmp/funk/./', needed by `/tmp/funk/atmsar11.fw'. Stop. make: *** [firmware_install] Error 2 /scratch/chas/net-next relax.55% mkdir /tmp/funk /scratch/chas/net-next relax.56% make INSTALL_FW_PATH="/tmp/funk" firmware_install INSTALL /tmp/funk/atmsar11.fw MKDIR /tmp/funk/e100/ INSTALL /tmp/funk/e100/d101m_ucode.bin INSTALL /tmp/funk/e100/d101s_ucode.bin INSTALL /tmp/funk/e100/d102e_ucode.bin ... i am somewhat concerned that it appears to have an extra set of "'s in the path as well. how are you passing in INSTALL_FW_PATH? i can get the same result with: /scratch/chas/net-next relax.57% make 'INSTALL_FW_PATH="/tmp/funk"' firmware_install make[1]: *** No rule to make target `"/tmp/funk"/./', needed by `"/tmp/funk"/atmsar11.fw'. Stop. make: *** [firmware_install] Error 2 but it would be wrong to do it this way. -- 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 5/6] drivers/atm: Use DIV_ROUND_UP
In message <[EMAIL PROTECTED]>,Julia Lawall write
s:
>In each case below, I have followed the original semantics, but in
>drivers/atm/eni.c and drivers/atm/horizon.c, I have some doubts as to
>whether the original semantics is correct. In drivers/atm/eni.c, is the
>division intended to be by div or by div-1? In drivers/atm/horizon.c, it
>seems strange that "case round_down" is implemented by DIV_ROUND_UP, twice.
>The round_down and default (ie round_up) cases seem to be inversed.
i guess it might seem confusing. to round the cell rate down, you need
to round the clock period up for the hardware scheduler. so the up/down
refer to the atm rate, not the clock period which is what appears to be
computed in the following sections.
for example, in the eni driver, but comp_tx() is computing a clock
period. to round the desired cell rate down, the clock period is
rounded up.
>diff -u -p a/drivers/atm/eni.c b/drivers/atm/eni.c
>--- a/drivers/atm/eni.c 2007-07-20 15:28:28.0 +0200
>+++ b/drivers/atm/eni.c 2008-02-13 20:50:10.0 +0100
>@@ -1270,7 +1270,7 @@ static int comp_tx(struct eni_dev *eni_d
> if (*pre < 3) (*pre)++; /* else fail later */
> div = pre_div[*pre]*-*pcr;
> DPRINTK("max div %d\n",div);
>- *res = (TS_CLOCK+div-1)/div-1;
>+ *res = DIV_ROUND_UP(TS_CLOCK, div)-1;
> }
> if (*res < 0) *res = 0;
> if (*res > MID_SEG_MAX_RATE) *res = MID_SEG_MAX_RATE;
>diff -u -p a/drivers/atm/horizon.c b/drivers/atm/horizon.c
>--- a/drivers/atm/horizon.c 2007-11-08 18:33:26.0 +0100
>+++ b/drivers/atm/horizon.c 2008-02-13 20:50:13.0 +0100
>@@ -635,7 +635,7 @@ static int make_rate (const hrz_dev * de
> // take care of rounding
> switch (r) {
> case round_down:
>- pre = (br+(c<+ pre = DIV_ROUND_UP(br, c< // but p must be non-zero
> if (!pre)
> pre = 1;
>@@ -668,7 +668,7 @@ static int make_rate (const hrz_dev * de
> // take care of rounding
> switch (r) {
> case round_down:
>- pre = (br+(c<+ pre = DIV_ROUND_UP(br, c< break;
> case round_nearest:
> pre = (br+(c<@@ -698,7 +698,7 @@ got_it:
> if (bits)
> *bits = (div< if (actual) {
>- *actual = (br + (pre<+ *actual = DIV_ROUND_UP(br, pre< PRINTD (DBG_QOS, "actual rate: %u", *actual);
> }
> return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] atm/ambassador: kmalloc + memset conversion to kzalloc
In message <[EMAIL PROTECTED]>,"Rober t P. J. Day" writes: >> > in any event, i just thought i'd point it out. if you're absolutely >> > sure there will never be another call to setup_dev() from somewhere >> > else, then, yes, it's safe. >> >> I understood your opinions. and partially agree with you. >> But isn't it a unfounded fear? > >i don't know, i just thought i'd mention it. if no one thinks it's an >issue, it's certainly fine with me. its very unlikely that setup_dev() is likely to be called from another code path. this patch looks fine to me. i will take it and get it submitted on the next merge. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kernel headers - linux-atm userspace build broken by recent change; __be16 undefined
it might be that the userspace code shouldnt be including if_arp.h. can you try that instead? In message <[EMAIL PROTECTED]>,Andrew Walrond writes: >Don't know exactly when this change went in, but it's not in 2.6.18.3 >and is in 2.6.19.2+ > > $ diff linux/include/linux/if_arp.h linux-2.6/include/linux/if_arp.h >133,134c133,134 >< unsigned short ar_hrd; /* format of hardware address */ >< unsigned short ar_pro; /* format of protocol address */ >--- > > __be16 ar_hrd; /* format of hardware address */ > > __be16 ar_pro; /* format of protocol address */ >137c137 >< unsigned short ar_op; /* ARP opcode (command) */ >--- > > __be16 ar_op; /* ARP opcode (command) */ > > >This causes the linux-atm userspace compile to fail like this: > >In file included from arp.c:19: >/usr/include/linux/if_arp.h:133: error: expected >specifier-qualifier-list before '__be16' > >I guess if_arp.h needs to include include/linux/byteorder/big_endian.h? > >Andrew Walrond - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2.6 patch] fix bridge <-> ATM compile error
In message <[EMAIL PROTECTED]>,Adrian Bunk writes:
>Letting CONFIG_BRIDGE depend on CONFIG_ATM doesn't sound like a good
>idea, since I doubt all people using the Bridge code require ATM
>support.
how about the following?
= net/atm/common.c 1.58 vs edited =
--- 1.58/net/atm/common.c 2005-01-20 21:17:39 -05:00
+++ edited/net/atm/common.c 2005-03-16 12:44:37 -05:00
@@ -755,21 +755,6 @@
return vcc->dev->ops->getsockopt(vcc, level, optname, optval, len);
}
-
-#if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
-#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
-struct net_bridge;
-struct net_bridge_fdb_entry *(*br_fdb_get_hook)(struct net_bridge *br,
- unsigned char *addr) = NULL;
-void (*br_fdb_put_hook)(struct net_bridge_fdb_entry *ent) = NULL;
-#if defined(CONFIG_ATM_LANE_MODULE) || defined(CONFIG_BRIDGE_MODULE)
-EXPORT_SYMBOL(br_fdb_get_hook);
-EXPORT_SYMBOL(br_fdb_put_hook);
-#endif /* defined(CONFIG_ATM_LANE_MODULE) || defined(CONFIG_BRIDGE_MODULE) */
-#endif /* defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) */
-#endif /* defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE) */
-
-
static int __init atm_init(void)
{
int error;
= net/atm/lec.c 1.47 vs edited =
--- 1.47/net/atm/lec.c 2005-02-08 23:09:15 -05:00
+++ edited/net/atm/lec.c2005-03-16 13:18:28 -05:00
@@ -37,11 +37,8 @@
#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
#include
#include "../bridge/br_private.h"
-static unsigned char bridge_ula_lec[] = {0x01, 0x80, 0xc2, 0x00, 0x00};
-extern struct net_bridge_fdb_entry *(*br_fdb_get_hook)(struct net_bridge *br,
- unsigned char *addr);
-extern void (*br_fdb_put_hook)(struct net_bridge_fdb_entry *ent);
+static unsigned char bridge_ula_lec[] = {0x01, 0x80, 0xc2, 0x00, 0x00};
#endif
/* Modular too */
= net/atm/lec.h 1.13 vs edited =
--- 1.13/net/atm/lec.h 2005-01-18 15:59:15 -05:00
+++ edited/net/atm/lec.h2005-03-16 13:22:13 -05:00
@@ -14,14 +14,6 @@
#include
#include
-#if defined (CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
-#include
-struct net_bridge;
-extern struct net_bridge_fdb_entry *(*br_fdb_get_hook)(struct net_bridge *br,
-unsigned char *addr);
-extern void (*br_fdb_put_hook)(struct net_bridge_fdb_entry *ent);
-#endif /* defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) */
-
#define LEC_HEADER_LEN 16
struct lecdatahdr_8023 {
= net/bridge/br.c 1.20 vs edited =
--- 1.20/net/bridge/br.c2005-03-10 21:50:08 -05:00
+++ edited/net/bridge/br.c 2005-03-16 13:21:27 -05:00
@@ -22,10 +22,6 @@
#include "br_private.h"
-#if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
-#include "../atm/lec.h"
-#endif
-
int (*br_should_route_hook) (struct sk_buff **pskb) = NULL;
static int __init br_init(void)
@@ -39,10 +35,9 @@
brioctl_set(br_ioctl_deviceless_stub);
br_handle_frame_hook = br_handle_frame;
-#if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
br_fdb_get_hook = br_fdb_get;
br_fdb_put_hook = br_fdb_put;
-#endif
+
register_netdevice_notifier(&br_device_notifier);
return 0;
@@ -60,10 +55,8 @@
synchronize_net();
-#if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
br_fdb_get_hook = NULL;
br_fdb_put_hook = NULL;
-#endif
br_handle_frame_hook = NULL;
br_fdb_fini();
= net/bridge/br_private.h 1.37 vs edited =
--- 1.37/net/bridge/br_private.h2005-03-10 21:51:37 -05:00
+++ edited/net/bridge/br_private.h 2005-03-16 13:19:34 -05:00
@@ -216,6 +216,12 @@
extern void br_stp_port_timer_init(struct net_bridge_port *p);
extern unsigned long br_timer_value(const struct timer_list *timer);
+/* br.c */
+extern struct net_bridge_fdb_entry *(*br_fdb_get_hook)(struct net_bridge *br,
+ unsigned char *addr);
+extern void (*br_fdb_put_hook)(struct net_bridge_fdb_entry *ent);
+
+
#ifdef CONFIG_SYSFS
/* br_sysfs_if.c */
extern int br_sysfs_addif(struct net_bridge_port *p);
= net/core/dev.c 1.185 vs edited =
--- 1.185/net/core/dev.c2005-02-11 00:54:32 -05:00
+++ edited/net/core/dev.c 2005-03-16 13:29:23 -05:00
@@ -1561,6 +1561,10 @@
#if defined(CONFIG_BRIDGE) || defined (CONFIG_BRIDGE_MODULE)
int (*br_handle_frame_hook)(struct net_bridge_port *p, struct sk_buff **pskb);
+struct net_bridge;
+struct net_bridge_fdb_entry *(*br_fdb_get_hook)(struct net_bridge *br,
+ unsigned char *addr);
+void (*br_fdb_put_hook)(struct net_bridge_fdb_entry *ent);
static __inline__ int handle_bridge(struct sk_buff **pskb,
struct packet_type **pt_prev, int *ret)
@@ -3346,6 +3350,8 @@
#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
EXPORT_SYMBOL(
Re: [2.6 patch] fix bridge <-> ATM compile error
In message <[EMAIL PROTECTED]>,Adrian Bunk writes: >Letting CONFIG_BRIDGE depend on CONFIG_ATM doesn't sound like a good >idea, since I doubt all people using the Bridge code require ATM >support. i agree. >Moving the hooks to the bridge code will give you exactly the same >problems the other way round. how about moving them to a third location like net/core/dev.c? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2.6 patch] fix bridge <-> ATM compile error
In message <[EMAIL PROTECTED]>,Adrian Bunk writes: >This patch fixes the following compile error with CONFIG_BRIDGE=y and >CONFIG_ATM_LANE=m: isnt the problem more that CONFIG_ATM=m not CONFIG_ATM_LANE=m? perhaps CONFIG_BRIDGE should be dependent on CONFIG_ATM. if atm is a module then bridge cannot be a module (unless the hooks are moved from atm to bridge)? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-ATM-General] Kernel 2.6.10 and 2.4.29 Oops fore200e (fwd)
In message <[EMAIL PROTECTED]>,Lukasz Trabinski writes: >OK, I think that dirver works much better with udelay() function. good to hear. what does atmdiag say about that interface? does it have a large percentage of tx drops? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-ATM-General] Kernel 2.6.10 and 2.4.29 Oops fore200e (fwd)
the author sent me the latest version of the driver and i
got it applied. the driver does has some useful changes
along with this broken change. i suggest udelay() since
it preserves the author's original intent.
i intend to submit a patch this week. i probably wont
fix the ambassador since i cant test the change.
In message <[EMAIL PROTECTED]>,Mike Westall writes:
>You could also just revert to kernel 2.4.25 or
>earlier. Someone who was apparently oblivious
>to the fact that device driver send routines
>were "routinely" called in irq context and/or
>that it was a to call schedule()
>under such circumstances slipped that one in
>sometime between 2.4.25 which is OK and 2.4.28
>where it is broken.
>
>In 2.4.25 and earlier it was a simple busy wait loop
>in which "goto retry_here;" immediately followed
>the "if" statement. This was safe, albeit MP unfriendly
>because of the spin_lock()/unlock() on each iteration.
>
>I'd say just delete the if and drop the damn
>packet.
>
>At any rate someone who has access to the golden code
>should fix this one way or another ASAP because its
>definitely seriously broken the way it is now.
>
>Mike
>
>
>chas williams - CONTRACTOR wrote:
>> In message <[EMAIL PROTECTED]>,Lukasz Trabinsk
>> i writes:
>>
>>>Sorry, but I don;t understand, what line, i am not kernel guru. :/
>>
>>
>> look for the following code:
>>
>>/* retry once again? */
>> if(--retry > 0) {
>> schedule();
>> goto retry_here;
>> }
>>
>>
>> change schedule() to udelay(50) and see if things are 'better'.
>>
>>
>>>Is was happened on 2.4.29, too. It is a interrupt problem?
>>
>>
>> its calling a routine that might sleep while in the transmit routine.
>> this is not allow.
>>
>>
>> ---
>> This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
>> Tool for open source databases. Create drag-&-drop reports. Save time
>> by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
>> Download a FREE copy at http://www.intelliview.com/go/osdn_nl
>> ___
>> Linux-atm-general mailing list
>> [EMAIL PROTECTED]
>> https://lists.sourceforge.net/lists/listinfo/linux-atm-general
>>
>>
>
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-ATM-General] Kernel 2.6.10 and 2.4.29 Oops fore200e (fwd)
In message <[EMAIL PROTECTED]>,Lukasz Trabinsk
i writes:
>Sorry, but I don;t understand, what line, i am not kernel guru. :/
look for the following code:
/* retry once again? */
if(--retry > 0) {
schedule();
goto retry_here;
}
change schedule() to udelay(50) and see if things are 'better'.
>Is was happened on 2.4.29, too. It is a interrupt problem?
its calling a routine that might sleep while in the transmit routine.
this is not allow.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
