RE: [EXT] [PATCH 2/2] qede: Use 'skb_add_rx_frag()' instead of hand coding it

2021-04-05 Thread Manish Chopra 
> -Original Message-
> From: Christophe JAILLET 
> Sent: Sunday, April 4, 2021 6:13 PM
> To: Ariel Elior ; GR-everest-linux-l2  l...@marvell.com>; da...@davemloft.net; k...@kernel.org
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> janit...@vger.kernel.org; Christophe JAILLET 
> Subject: [EXT] [PATCH 2/2] qede: Use 'skb_add_rx_frag()' instead of hand 
> coding
> it
> 
> External Email
> 
> --
> Some lines of code can be merged into an equivalent 'skb_add_rx_frag()'
> call which is less verbose.
> 
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/net/ethernet/qlogic/qede/qede_fp.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index ee3e45e38cb7..8e150dd4f899 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> @@ -1209,12 +1209,9 @@ static int qede_rx_build_jumbo(struct qede_dev
> *edev,
>   dma_unmap_page(rxq->dev, bd->mapping,
>  PAGE_SIZE, DMA_FROM_DEVICE);
> 
> - skb_fill_page_desc(skb, skb_shinfo(skb)->nr_frags,
> -bd->data, rxq->rx_headroom, cur_size);
> + skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, bd->data,
> + rxq->rx_headroom, cur_size, PAGE_SIZE);
> 
> - skb->truesize += PAGE_SIZE;
> - skb->data_len += cur_size;
> - skb->len += cur_size;
>   pkt_len -= cur_size;
>   }
> 
> --
> 2.27.0

Thank you Christophe.
Acked-by: Manish Chopra

RE: [EXT] [PATCH 1/2] qede: Remove a erroneous ++ in 'qede_rx_build_jumbo()'

2021-04-05 Thread Manish Chopra 
> -Original Message-
> From: Christophe JAILLET 
> Sent: Sunday, April 4, 2021 6:13 PM
> To: Ariel Elior ; GR-everest-linux-l2  l...@marvell.com>; da...@davemloft.net; k...@kernel.org
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> janit...@vger.kernel.org; Christophe JAILLET 
> Subject: [EXT] [PATCH 1/2] qede: Remove a erroneous ++ in
> 'qede_rx_build_jumbo()'
> 
> External Email
> 
> --
> This ++ is confusing. It looks duplicated with the one already performed in
> 'skb_fill_page_desc()'.
> 
> In fact, it is harmless. 'nr_frags' is written twice with the same value.
> Once, because of the nr_frags++, and once because of the 'nr_frags = i + 1'
> in 'skb_fill_page_desc()'.
> 
> So axe this post-increment to avoid confusion.
> 
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/net/ethernet/qlogic/qede/qede_fp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index 102d0e0808d5..ee3e45e38cb7 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> @@ -1209,7 +1209,7 @@ static int qede_rx_build_jumbo(struct qede_dev
> *edev,
>   dma_unmap_page(rxq->dev, bd->mapping,
>  PAGE_SIZE, DMA_FROM_DEVICE);
> 
> - skb_fill_page_desc(skb, skb_shinfo(skb)->nr_frags++,
> + skb_fill_page_desc(skb, skb_shinfo(skb)->nr_frags,
>      bd->data, rxq->rx_headroom, cur_size);
> 
>   skb->truesize += PAGE_SIZE;
> --
> 2.27.0

Acked-by: Manish Chopra

RE: [PATCH] qlge: Replace create_singlethread_workqueue with alloc_ordered_workqueue

2016-04-14 Thread Manish Chopra 
> -Original Message-
> From: dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com
> [mailto:dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com] On Behalf
> Of Tejun Heo
> Sent: Wednesday, April 13, 2016 11:14 PM
> To: Amitoj Kaur Chawla 
> Cc: Sudarsana Kalluru ; netdev
> ; linux-kernel ; Dept-
> Eng Linux Driver ; Harish Patil
> ; Dept-GE Linux NIC Dev  gelinuxnic...@qlogic.com>
> Subject: Re: [PATCH] qlge: Replace create_singlethread_workqueue with
> alloc_ordered_workqueue
> 
> On Sat, Apr 09, 2016 at 05:27:45PM +0530, Amitoj Kaur Chawla wrote:
> > Replace deprecated create_singlethread_workqueue with
> > alloc_ordered_workqueue.
> >
> > Work items include getting tx/rx frame sizes, resetting MPI processor,
> > setting asic recovery bit so ordering seems necessary as only one work
> > item should be in queue/executing at any given time, hence the use of
> > alloc_ordered_workqueue.
> >
> > WQ_MEM_RECLAIM flag has been set since ethernet devices seem to sit in
> > memory reclaim path, so to guarantee forward progress regardless of
> > memory pressure.
> >
> > Signed-off-by: Amitoj Kaur Chawla 
> > Acked-by: Tejun Heo 
> 
> Ping?
> 

Hi,
Just want to confirm that __WQ_LEGACY flag is not necessary here as this is 
removed
with this change ? 

Thanks

RE: [PATCH] qlge: Replace create_singlethread_workqueue with alloc_ordered_workqueue

2016-04-14 Thread Manish Chopra 
> -Original Message-
> From: dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com
> [mailto:dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com] On Behalf
> Of Tejun Heo
> Sent: Wednesday, April 13, 2016 11:14 PM
> To: Amitoj Kaur Chawla 
> Cc: Sudarsana Kalluru ; netdev
> ; linux-kernel ; Dept-
> Eng Linux Driver ; Harish Patil
> ; Dept-GE Linux NIC Dev  gelinuxnic...@qlogic.com>
> Subject: Re: [PATCH] qlge: Replace create_singlethread_workqueue with
> alloc_ordered_workqueue
> 
> On Sat, Apr 09, 2016 at 05:27:45PM +0530, Amitoj Kaur Chawla wrote:
> > Replace deprecated create_singlethread_workqueue with
> > alloc_ordered_workqueue.
> >
> > Work items include getting tx/rx frame sizes, resetting MPI processor,
> > setting asic recovery bit so ordering seems necessary as only one work
> > item should be in queue/executing at any given time, hence the use of
> > alloc_ordered_workqueue.
> >
> > WQ_MEM_RECLAIM flag has been set since ethernet devices seem to sit in
> > memory reclaim path, so to guarantee forward progress regardless of
> > memory pressure.
> >
> > Signed-off-by: Amitoj Kaur Chawla 
> > Acked-by: Tejun Heo 
> 
> Ping?
> 

Hi,
Just want to confirm that __WQ_LEGACY flag is not necessary here as this is 
removed
with this change ? 

Thanks

RE: [PATCH RESEND] bnx2x:Add proper protection from concurrent users in the function bnx2_open

2015-09-25 Thread Manish Chopra 
> -Original Message-
> From: dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com
> [mailto:dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com] On Behalf
> Of Nicholas Krause
> Sent: Friday, September 25, 2015 3:55 AM
> To: Sony Chacko
> Cc: Dept-GE Linux NIC Dev; linux-kernel; netdev
> Subject: [PATCH RESEND] bnx2x:Add proper protection from concurrent users in
> the function bnx2_open
> 
> This fixes bnx2_open to have proper protection from concurrent users as it is
> never properly locked with rtnl_lock/unlock before executing its code that can
> have issues with other threads of execution executing on these data structures
> at the same time. Due to this fix it by making this locking internal to the 
> function
> bnx2_open by unlocking before and after the critical region with the function
> pair rtnl_lock/unlock.
> 
> Signed-off-by: Nicholas Krause 
> ---
>  drivers/net/ethernet/broadcom/bnx2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c
> b/drivers/net/ethernet/broadcom/bnx2.c
> index 2b66ef3..0c31c49 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -6331,13 +6331,13 @@ bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
>   return netif_set_real_num_rx_queues(bp->dev, bp->num_rx_rings);  }
> 
> -/* Called with rtnl_lock */
>  static int
>  bnx2_open(struct net_device *dev)
>  {
>   struct bnx2 *bp = netdev_priv(dev);
>   int rc;
> 
> + rtnl_lock();
>   rc = bnx2_request_firmware(bp);
>   if (rc < 0)
>   goto out;
> @@ -6411,6 +6411,7 @@ open_err:
>   bnx2_free_mem(bp);
>   bnx2_del_napi(bp);
>   bnx2_release_firmware(bp);
> + rtnl_unlock();
>   goto out;
>  }
> 
> --

It's not an appropriate change. Acquiring rtnl_lock() in device's ndo_open() 
handler will cause deadlock
as OS already take this lock before calling ndo_open().

Also, the subject line is not correct - it was supposed to be bnx2 change, not 
bnx2x.

Thanks,
Manish

--
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 RESEND] bnx2x:Add proper protection from concurrent users in the function bnx2_open

2015-09-25 Thread Manish Chopra 
> -Original Message-
> From: dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com
> [mailto:dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com] On Behalf
> Of Nicholas Krause
> Sent: Friday, September 25, 2015 3:55 AM
> To: Sony Chacko
> Cc: Dept-GE Linux NIC Dev; linux-kernel; netdev
> Subject: [PATCH RESEND] bnx2x:Add proper protection from concurrent users in
> the function bnx2_open
> 
> This fixes bnx2_open to have proper protection from concurrent users as it is
> never properly locked with rtnl_lock/unlock before executing its code that can
> have issues with other threads of execution executing on these data structures
> at the same time. Due to this fix it by making this locking internal to the 
> function
> bnx2_open by unlocking before and after the critical region with the function
> pair rtnl_lock/unlock.
> 
> Signed-off-by: Nicholas Krause 
> ---
>  drivers/net/ethernet/broadcom/bnx2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c
> b/drivers/net/ethernet/broadcom/bnx2.c
> index 2b66ef3..0c31c49 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -6331,13 +6331,13 @@ bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
>   return netif_set_real_num_rx_queues(bp->dev, bp->num_rx_rings);  }
> 
> -/* Called with rtnl_lock */
>  static int
>  bnx2_open(struct net_device *dev)
>  {
>   struct bnx2 *bp = netdev_priv(dev);
>   int rc;
> 
> + rtnl_lock();
>   rc = bnx2_request_firmware(bp);
>   if (rc < 0)
>   goto out;
> @@ -6411,6 +6411,7 @@ open_err:
>   bnx2_free_mem(bp);
>   bnx2_del_napi(bp);
>   bnx2_release_firmware(bp);
> + rtnl_unlock();
>   goto out;
>  }
> 
> --

It's not an appropriate change. Acquiring rtnl_lock() in device's ndo_open() 
handler will cause deadlock
as OS already take this lock before calling ndo_open().

Also, the subject line is not correct - it was supposed to be bnx2 change, not 
bnx2x.

Thanks,
Manish

--
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] netxen_nic: use spin_[un]lock_bh around tx_clean_lock (2)

2015-05-07 Thread Manish Chopra 
> -Original Message-
> From: Tony Camuso [mailto:tcam...@redhat.com]
> Sent: Wednesday, May 06, 2015 6:39 PM
> To: linux-kernel; netdev
> Cc: Manish Chopra; Sony Chacko; Rajesh Borundia; nhor...@redhat.com
> Subject: [PATCH] netxen_nic: use spin_[un]lock_bh around tx_clean_lock (2)
> 
> This patch should have been part of the previous patch having the same
> summary. See  http://marc.info/?l=linux-kernel=143039470103795=2
> Unfortunately, I didn't check to see where else this lock was used before
> submitting that patch. This should take care of it for netxen_nic, as I did a
> thorough search this time.
> 
> To recap from the original patch; although testing this driver with
> DEBUG_LOCKDEP and DEBUG_SPINLOCK enabled did not produce any traces, it
> would be more prudent in the case of tx_clean_lock to use _bh versions of
> spin_[un]lock, since this lock is manipulated in both the process and softirq
> contexts.
> 
> This patch was tested for functionality and regressions with netperf and
> DEBUG_LOCKDEP and DEBUG_SPINLOCK enabled.
> 
> Signed-off-by: Tony Camuso 
> ---
>  drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> index 2da9627..6301bae 100644
> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> @@ -1766,7 +1766,7 @@ int netxen_process_cmd_ring(struct netxen_adapter
> *adapter)
>   int done = 0;
>   struct nx_host_tx_ring *tx_ring = adapter->tx_ring;
> 
> - if (!spin_trylock(>tx_clean_lock))
> + if (!spin_trylock_bh(>tx_clean_lock))
>   return 1;
> 
>   sw_consumer = tx_ring->sw_consumer;
> @@ -1821,7 +1821,7 @@ int netxen_process_cmd_ring(struct netxen_adapter
> *adapter)
>*/
>   hw_consumer = le32_to_cpu(*(tx_ring->hw_consumer));
>   done = (sw_consumer == hw_consumer);
> - spin_unlock(>tx_clean_lock);
> + spin_unlock_bh(>tx_clean_lock);
> 
>   return done;
>  }
> --
> 1.8.3.1

Acked-By: Manish Chopra 
--
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] netxen_nic: use spin_[un]lock_bh around tx_clean_lock (2)

2015-05-07 Thread Manish Chopra 
 -Original Message-
 From: Tony Camuso [mailto:tcam...@redhat.com]
 Sent: Wednesday, May 06, 2015 6:39 PM
 To: linux-kernel; netdev
 Cc: Manish Chopra; Sony Chacko; Rajesh Borundia; nhor...@redhat.com
 Subject: [PATCH] netxen_nic: use spin_[un]lock_bh around tx_clean_lock (2)
 
 This patch should have been part of the previous patch having the same
 summary. See  http://marc.info/?l=linux-kernelm=143039470103795w=2
 Unfortunately, I didn't check to see where else this lock was used before
 submitting that patch. This should take care of it for netxen_nic, as I did a
 thorough search this time.
 
 To recap from the original patch; although testing this driver with
 DEBUG_LOCKDEP and DEBUG_SPINLOCK enabled did not produce any traces, it
 would be more prudent in the case of tx_clean_lock to use _bh versions of
 spin_[un]lock, since this lock is manipulated in both the process and softirq
 contexts.
 
 This patch was tested for functionality and regressions with netperf and
 DEBUG_LOCKDEP and DEBUG_SPINLOCK enabled.
 
 Signed-off-by: Tony Camuso tcam...@redhat.com
 ---
  drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
 b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
 index 2da9627..6301bae 100644
 --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
 +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
 @@ -1766,7 +1766,7 @@ int netxen_process_cmd_ring(struct netxen_adapter
 *adapter)
   int done = 0;
   struct nx_host_tx_ring *tx_ring = adapter-tx_ring;
 
 - if (!spin_trylock(adapter-tx_clean_lock))
 + if (!spin_trylock_bh(adapter-tx_clean_lock))
   return 1;
 
   sw_consumer = tx_ring-sw_consumer;
 @@ -1821,7 +1821,7 @@ int netxen_process_cmd_ring(struct netxen_adapter
 *adapter)
*/
   hw_consumer = le32_to_cpu(*(tx_ring-hw_consumer));
   done = (sw_consumer == hw_consumer);
 - spin_unlock(adapter-tx_clean_lock);
 + spin_unlock_bh(adapter-tx_clean_lock);
 
   return done;
  }
 --
 1.8.3.1

Acked-By: Manish Chopra manish.cho...@qlogic.com
--
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: netxen: BUG: sleeping function called from invalid context at include/linux/netdevice.h:476

2014-09-25 Thread Manish Chopra 
> -Original Message-
> From: Mike Galbraith [mailto:umgwanakikb...@gmail.com]
> Sent: Thursday, September 25, 2014 7:52 PM
> To: linux-kernel
> Cc: Manish Chopra; Sony Chacko; Rajesh Borundia; netdev
> Subject: netxen: BUG: sleeping function called from invalid context at
> include/linux/netdevice.h:476
> 
> Greetings,
> 
> While testing some sched patches that require CONFIG_DEBUG_ATOMIC_SLEEP,
> the below fell out.
> 
> /etc/init.d/network restart -> gripe
> 
> [   97.850408] BUG: sleeping function called from invalid context at
> include/linux/netdevice.h:476
> [   97.941177] in_atomic(): 1, irqs_disabled(): 0, pid: 6458, name: ip
> [   98.012784] Preemption disabled at:[]
> __netxen_nic_down+0x79/0x1b0 [netxen_nic]
> [   98.047283]
> [   98.047289] CPU: 63 PID: 6458 Comm: ip Tainted: GE  
> 3.17.0-default #7
> [   98.047291] Hardware name: Hewlett-Packard ProLiant DL980 G7, BIOS P66
> 07/07/2010
> [   98.047295]   88003702f528 81589dbd
> 
> [   98.047301]  88026ee96210 88003702f548 8108e1df
> 88026ee96210
> [   98.047305]   88003702f578 8108e298
> 88026bf89c38
> [   98.047307] Call Trace:
> [   98.047313]  [] dump_stack+0x4d/0x90
> [   98.047331]  [] ___might_sleep+0x10f/0x180
> [   98.047343]  [] __might_sleep+0x48/0xd0
> [   98.047348]  [] netxen_napi_disable+0x84/0xe0
> [netxen_nic]
> [   98.047353]  [] __netxen_nic_down+0x150/0x1b0
> [netxen_nic]
> [   98.047367]  [] netxen_nic_close+0x1b/0x20 [netxen_nic]
> [   98.047375]  [] __dev_close_many+0x95/0xe0
> [   98.047379]  [] __dev_close+0x36/0x50
> [   98.047384]  [] __dev_change_flags+0xac/0x180
> [   98.047389]  [] dev_change_flags+0x37/0x80
> [   98.047398]  [] do_setlink+0x244/0x7e0
> [   98.047403]  [] rtnl_newlink+0x5a0/0x7d0
> [   98.047408]  [] ? rtnl_newlink+0x14a/0x7d0
> [   98.047421]  [] ? __do_page_fault+0x2ac/0x520
> [   98.047431]  [] ? apparmor_capable+0x20/0x60
> [   98.047436]  [] rtnetlink_rcv_msg+0xa1/0x240
> [   98.047446]  [] ? rhashtable_lookup_compare+0x46/0x70
> [   98.047451]  [] ? __rtnl_unlock+0x20/0x20
> [   98.047463]  [] netlink_rcv_skb+0x89/0xb0
> [   98.047466]  [] rtnetlink_rcv+0x2c/0x40
> [   98.047468]  [] netlink_unicast+0x119/0x180
> [   98.047473]  [] ? memcpy_fromiovec+0x6c/0x90
> [   98.047477]  [] netlink_sendmsg+0x3c0/0x450
> [   98.047487]  [] sock_sendmsg+0x9c/0xd0
> [   98.047497]  [] ? __wake_up+0x53/0x70
> [   98.047502]  [] ? __might_sleep+0x48/0xd0
> [   98.047509]  [] ? verify_iovec+0x5e/0xf0
> [   98.047512]  [] ___sys_sendmsg+0x436/0x440
> [   98.047515]  [] ? __might_sleep+0x48/0xd0
> [   98.047523]  [] ? might_fault+0x43/0x50
> [   98.047525]  [] ? copy_to_user+0x2f/0x40
> [   98.047528]  [] ? ___sys_recvmsg+0x19c/0x310
> [   98.047531]  [] ? __do_page_fault+0x2ac/0x520
> [   98.047538]  [] ? __vma_link_rb+0x101/0x120
> [   98.047544]  [] ? do_brk+0x1c8/0x340
> [   98.047550]  [] ? SyS_getsockname+0xb2/0xc0
> [   98.047555]  [] __sys_sendmsg+0x49/0x80
> [   98.047558]  [] SyS_sendmsg+0x19/0x20
> [   98.047567]  [] system_call_fastpath+0x16/0x1b
> 
> (gdb) list *__netxen_nic_down+0x79
> 0x8749 is in __netxen_nic_down
> (drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c:1190).
> 1185if (!test_and_clear_bit(__NX_DEV_UP, >state))
> 1186return;
> 1187
> 1188smp_mb();
> 1189spin_lock(>tx_clean_lock);
> 1190netif_carrier_off(netdev);
> 1191netif_tx_disable(netdev);
> 1192
> 1193if (adapter->capabilities &
> NX_FW_CAPABILITY_LINK_NOTIFICATION)
> 1194netxen_linkevent_request(adapter, 0);

Hello Mike,

Thanks for finding this out. Mostly I have fix for it.
I will send a patch to net for fixing it after doing some testing.

Thanks,
Manish 


N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: netxen: BUG: sleeping function called from invalid context at include/linux/netdevice.h:476

2014-09-25 Thread Manish Chopra 
 -Original Message-
 From: Mike Galbraith [mailto:umgwanakikb...@gmail.com]
 Sent: Thursday, September 25, 2014 7:52 PM
 To: linux-kernel
 Cc: Manish Chopra; Sony Chacko; Rajesh Borundia; netdev
 Subject: netxen: BUG: sleeping function called from invalid context at
 include/linux/netdevice.h:476
 
 Greetings,
 
 While testing some sched patches that require CONFIG_DEBUG_ATOMIC_SLEEP,
 the below fell out.
 
 /etc/init.d/network restart - gripe
 
 [   97.850408] BUG: sleeping function called from invalid context at
 include/linux/netdevice.h:476
 [   97.941177] in_atomic(): 1, irqs_disabled(): 0, pid: 6458, name: ip
 [   98.012784] Preemption disabled at:[a0509719]
 __netxen_nic_down+0x79/0x1b0 [netxen_nic]
 [   98.047283]
 [   98.047289] CPU: 63 PID: 6458 Comm: ip Tainted: GE  
 3.17.0-default #7
 [   98.047291] Hardware name: Hewlett-Packard ProLiant DL980 G7, BIOS P66
 07/07/2010
 [   98.047295]   88003702f528 81589dbd
 
 [   98.047301]  88026ee96210 88003702f548 8108e1df
 88026ee96210
 [   98.047305]   88003702f578 8108e298
 88026bf89c38
 [   98.047307] Call Trace:
 [   98.047313]  [81589dbd] dump_stack+0x4d/0x90
 [   98.047331]  [8108e1df] ___might_sleep+0x10f/0x180
 [   98.047343]  [8108e298] __might_sleep+0x48/0xd0
 [   98.047348]  [a0506d54] netxen_napi_disable+0x84/0xe0
 [netxen_nic]
 [   98.047353]  [a05097f0] __netxen_nic_down+0x150/0x1b0
 [netxen_nic]
 [   98.047367]  [a0509b0b] netxen_nic_close+0x1b/0x20 [netxen_nic]
 [   98.047375]  [814aa525] __dev_close_many+0x95/0xe0
 [   98.047379]  [814aa5a6] __dev_close+0x36/0x50
 [   98.047384]  [814ab89c] __dev_change_flags+0xac/0x180
 [   98.047389]  [814ab9a7] dev_change_flags+0x37/0x80
 [   98.047398]  [814be874] do_setlink+0x244/0x7e0
 [   98.047403]  [814c0310] rtnl_newlink+0x5a0/0x7d0
 [   98.047408]  [814bfeba] ? rtnl_newlink+0x14a/0x7d0
 [   98.047421]  [8105259c] ? __do_page_fault+0x2ac/0x520
 [   98.047431]  [812a14a0] ? apparmor_capable+0x20/0x60
 [   98.047436]  [814bf981] rtnetlink_rcv_msg+0xa1/0x240
 [   98.047446]  [81308f86] ? rhashtable_lookup_compare+0x46/0x70
 [   98.047451]  [814bf8e0] ? __rtnl_unlock+0x20/0x20
 [   98.047463]  [814dc3d9] netlink_rcv_skb+0x89/0xb0
 [   98.047466]  [814bfb7c] rtnetlink_rcv+0x2c/0x40
 [   98.047468]  [814dbdc9] netlink_unicast+0x119/0x180
 [   98.047473]  [81306a3c] ? memcpy_fromiovec+0x6c/0x90
 [   98.047477]  [814dc7c0] netlink_sendmsg+0x3c0/0x450
 [   98.047487]  [8149449c] sock_sendmsg+0x9c/0xd0
 [   98.047497]  [810aa113] ? __wake_up+0x53/0x70
 [   98.047502]  [8108e298] ? __might_sleep+0x48/0xd0
 [   98.047509]  [814a204e] ? verify_iovec+0x5e/0xf0
 [   98.047512]  [81494df6] ___sys_sendmsg+0x436/0x440
 [   98.047515]  [8108e298] ? __might_sleep+0x48/0xd0
 [   98.047523]  [81194513] ? might_fault+0x43/0x50
 [   98.047525]  [81491eef] ? copy_to_user+0x2f/0x40
 [   98.047528]  [814951cc] ? ___sys_recvmsg+0x19c/0x310
 [   98.047531]  [8105259c] ? __do_page_fault+0x2ac/0x520
 [   98.047538]  [8119e681] ? __vma_link_rb+0x101/0x120
 [   98.047544]  [8119fef8] ? do_brk+0x1c8/0x340
 [   98.047550]  [814924a2] ? SyS_getsockname+0xb2/0xc0
 [   98.047555]  [81494fd9] __sys_sendmsg+0x49/0x80
 [   98.047558]  [81495029] SyS_sendmsg+0x19/0x20
 [   98.047567]  [8158f7a9] system_call_fastpath+0x16/0x1b
 
 (gdb) list *__netxen_nic_down+0x79
 0x8749 is in __netxen_nic_down
 (drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c:1190).
 1185if (!test_and_clear_bit(__NX_DEV_UP, adapter-state))
 1186return;
 1187
 1188smp_mb();
 1189spin_lock(adapter-tx_clean_lock);
 1190netif_carrier_off(netdev);
 1191netif_tx_disable(netdev);
 1192
 1193if (adapter-capabilities 
 NX_FW_CAPABILITY_LINK_NOTIFICATION)
 1194netxen_linkevent_request(adapter, 0);

Hello Mike,

Thanks for finding this out. Mostly I have fix for it.
I will send a patch to net for fixing it after doing some testing.

Thanks,
Manish 


N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH NET-NEXT V2] netxen: fix ethtool rx_dropped information in ethtool get_ethtool_stats()

2014-07-09 Thread Manish Chopra 
>-Original Message-
>From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
>On Behalf Of ethan zhao
>Sent: Wednesday, July 09, 2014 8:11 AM
>To: David Miller
>Cc: ba...@ti.com; netdev; linux-kernel; sriharsha.dev...@oracle.com;
>ethan.ker...@gmail.com; vaughan
>Subject: Re: [PATCH NET-NEXT V2] netxen: fix ethtool rx_dropped information in
>ethtool get_ethtool_stats()
>
>David,
>
>Please help to review and confirm this patch.
>
>Thanks,
>Ethan
>
>On 2014/7/8 21:57, Ethan Zhao wrote:
>> netxen driver has implemented netxen_nic_get_ethtool_stats()
>> interface, but it doesn't collect stats.rxdropped in driver, so we
>> will get different rx_dropped statistic information while using ifconfig and
>ethtool.
>> this patch fills stats.rxdropped field with data from net core with
>> dev_get_stats() just as ixgbe driver did for some of its stats.
>>
>> V2: only fix the stats.rxdropped field.
>>
>> Tested with last netxen 4.0.82
>> Compiled with net-next branch 3.16-rc2
>>
>> Signed-off-by: Ethan Zhao 
>> Tested-by: Sriharsha Yadagudde 
>> ---
>>   .../ethernet/qlogic/netxen/netxen_nic_ethtool.c|   34 
>> +-
>--
>>   1 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> index 4ca2c19..49e6a1b 100644
>> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> @@ -33,22 +33,30 @@
>>   #include "netxen_nic.h"
>>   #include "netxen_nic_hw.h"
>>
>> +enum {NETDEV_STATS, NETXEN_STATS};
>> +
>>   struct netxen_nic_stats {
>>  char stat_string[ETH_GSTRING_LEN];
>> +int type;
>>  int sizeof_stat;
>>  int stat_offset;
>>   };
>>
>> -#define NETXEN_NIC_STAT(m) sizeof(((struct netxen_adapter *)0)->m), \
>> +#define NETXEN_NIC_STAT(m)  NETXEN_STATS, \
>> +sizeof(((struct netxen_adapter *)0)->m), \
>>  offsetof(struct netxen_adapter, m)
>>
>> +#define NETXEN_NETDEV_STAT(m)   NETDEV_STATS, \
>> +sizeof(((struct rtnl_link_stats64 *)0)->m), \
>> +offsetof(struct rtnl_link_stats64, m)
>> +
>>   #define NETXEN_NIC_PORT_WINDOW 0x1
>>   #define NETXEN_NIC_INVALID_DATA 0xDEADBEEF
>>
>>   static const struct netxen_nic_stats netxen_nic_gstrings_stats[] = {
>>  {"xmit_called", NETXEN_NIC_STAT(stats.xmitcalled)},
>>  {"xmit_finished", NETXEN_NIC_STAT(stats.xmitfinished)},
>> -{"rx_dropped", NETXEN_NIC_STAT(stats.rxdropped)},
>> +{"rx_dropped", NETXEN_NETDEV_STAT(rx_dropped)},
>>  {"tx_dropped", NETXEN_NIC_STAT(stats.txdropped)},
>>  {"csummed", NETXEN_NIC_STAT(stats.csummed)},
>>  {"rx_pkts", NETXEN_NIC_STAT(stats.rx_pkts)}, @@ -679,11 +687,27 @@
>> netxen_nic_get_ethtool_stats(struct net_device *dev,
>>   {
>>  struct netxen_adapter *adapter = netdev_priv(dev);
>>  int index;
>> +struct rtnl_link_stats64 temp;
>> +const struct rtnl_link_stats64 *net_stats;
>> +char *p = NULL;
>>
>> +net_stats = dev_get_stats(dev, );
>>  for (index = 0; index < NETXEN_NIC_STATS_LEN; index++) {
>> -char *p =
>> -(char *)adapter +
>> -netxen_nic_gstrings_stats[index].stat_offset;
>> +
>> +switch (netxen_nic_gstrings_stats[index].type) {
>> +case NETDEV_STATS:
>> +p = (char *)net_stats +
>> +netxen_nic_gstrings_stats[index].stat_offset;
>> +break;
>> +case NETXEN_STATS:
>> +p = (char *)adapter +
>> +netxen_nic_gstrings_stats[index].stat_offset;
>> +break;
>> +default:
>> +data[index] = 0;
>> +continue;
>> +}
>> +
>>  data[index] =
>>  (netxen_nic_gstrings_stats[index].sizeof_stat ==
>>   sizeof(u64)) ? *(u64 *) p : *(u32 *) p;

Ethan, I can't download this patch as I don’t see it in netdev patchwork. Just 
looking at the diff I think there is style issue with macro definition of 
NETXEN_NIC_STAT and NETXEN_NETDEV_STAT.
Enclose macro definition in parentheses as they are containing complex values.

Send this patch targeted for net instead of net-next being a bug fix. Please 
also run checkpatch.pl to correct any style warning/errors in the patch.

Thanks!!

RE: [PATCH NET-NEXT V2] netxen: fix ethtool rx_dropped information in ethtool get_ethtool_stats()

2014-07-09 Thread Manish Chopra 
-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
On Behalf Of ethan zhao
Sent: Wednesday, July 09, 2014 8:11 AM
To: David Miller
Cc: ba...@ti.com; netdev; linux-kernel; sriharsha.dev...@oracle.com;
ethan.ker...@gmail.com; vaughan
Subject: Re: [PATCH NET-NEXT V2] netxen: fix ethtool rx_dropped information in
ethtool get_ethtool_stats()

David,

Please help to review and confirm this patch.

Thanks,
Ethan

On 2014/7/8 21:57, Ethan Zhao wrote:
 netxen driver has implemented netxen_nic_get_ethtool_stats()
 interface, but it doesn't collect stats.rxdropped in driver, so we
 will get different rx_dropped statistic information while using ifconfig and
ethtool.
 this patch fills stats.rxdropped field with data from net core with
 dev_get_stats() just as ixgbe driver did for some of its stats.

 V2: only fix the stats.rxdropped field.

 Tested with last netxen 4.0.82
 Compiled with net-next branch 3.16-rc2

 Signed-off-by: Ethan Zhao ethan.z...@oracle.com
 Tested-by: Sriharsha Yadagudde sriharsha.dev...@oracle.com
 ---
   .../ethernet/qlogic/netxen/netxen_nic_ethtool.c|   34 
 +-
--
   1 files changed, 29 insertions(+), 5 deletions(-)

 diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
 b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
 index 4ca2c19..49e6a1b 100644
 --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
 +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
 @@ -33,22 +33,30 @@
   #include netxen_nic.h
   #include netxen_nic_hw.h

 +enum {NETDEV_STATS, NETXEN_STATS};
 +
   struct netxen_nic_stats {
  char stat_string[ETH_GSTRING_LEN];
 +int type;
  int sizeof_stat;
  int stat_offset;
   };

 -#define NETXEN_NIC_STAT(m) sizeof(((struct netxen_adapter *)0)-m), \
 +#define NETXEN_NIC_STAT(m)  NETXEN_STATS, \
 +sizeof(((struct netxen_adapter *)0)-m), \
  offsetof(struct netxen_adapter, m)

 +#define NETXEN_NETDEV_STAT(m)   NETDEV_STATS, \
 +sizeof(((struct rtnl_link_stats64 *)0)-m), \
 +offsetof(struct rtnl_link_stats64, m)
 +
   #define NETXEN_NIC_PORT_WINDOW 0x1
   #define NETXEN_NIC_INVALID_DATA 0xDEADBEEF

   static const struct netxen_nic_stats netxen_nic_gstrings_stats[] = {
  {xmit_called, NETXEN_NIC_STAT(stats.xmitcalled)},
  {xmit_finished, NETXEN_NIC_STAT(stats.xmitfinished)},
 -{rx_dropped, NETXEN_NIC_STAT(stats.rxdropped)},
 +{rx_dropped, NETXEN_NETDEV_STAT(rx_dropped)},
  {tx_dropped, NETXEN_NIC_STAT(stats.txdropped)},
  {csummed, NETXEN_NIC_STAT(stats.csummed)},
  {rx_pkts, NETXEN_NIC_STAT(stats.rx_pkts)}, @@ -679,11 +687,27 @@
 netxen_nic_get_ethtool_stats(struct net_device *dev,
   {
  struct netxen_adapter *adapter = netdev_priv(dev);
  int index;
 +struct rtnl_link_stats64 temp;
 +const struct rtnl_link_stats64 *net_stats;
 +char *p = NULL;

 +net_stats = dev_get_stats(dev, temp);
  for (index = 0; index  NETXEN_NIC_STATS_LEN; index++) {
 -char *p =
 -(char *)adapter +
 -netxen_nic_gstrings_stats[index].stat_offset;
 +
 +switch (netxen_nic_gstrings_stats[index].type) {
 +case NETDEV_STATS:
 +p = (char *)net_stats +
 +netxen_nic_gstrings_stats[index].stat_offset;
 +break;
 +case NETXEN_STATS:
 +p = (char *)adapter +
 +netxen_nic_gstrings_stats[index].stat_offset;
 +break;
 +default:
 +data[index] = 0;
 +continue;
 +}
 +
  data[index] =
  (netxen_nic_gstrings_stats[index].sizeof_stat ==
   sizeof(u64)) ? *(u64 *) p : *(u32 *) p;

Ethan, I can't download this patch as I don’t see it in netdev patchwork. Just 
looking at the diff I think there is style issue with macro definition of 
NETXEN_NIC_STAT and NETXEN_NETDEV_STAT.
Enclose macro definition in parentheses as they are containing complex values.

Send this patch targeted for net instead of net-next being a bug fix. Please 
also run checkpatch.pl to correct any style warning/errors in the patch.

Thanks!!