* Re: [PATCH] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Claudiu Manoil @ 2013-09-18 15:44 UTC (permalink / raw)
To: Aida Mynzhasova; +Cc: richardcochran, netdev
In-Reply-To: <1379510464-25420-1-git-send-email-aida.mynzhasova@skitlab.ru>
On 9/18/2013 4:21 PM, Aida Mynzhasova wrote:
> IEEE 1588 timer reference clock source is determined through hard-coded
> value in gianfar_ptp driver by default. This patch allows to select ptp
> clock source by means of device tree file node.
>
> For instance:
>
> fsl,cksel = <0>;
>
Has this device tree binding been defined? (Where?)
I don't see this property in the net-next.git tree at least.
Claudiu
^ permalink raw reply
* Re: [PATCH 005/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Joe Perches @ 2013-09-18 15:42 UTC (permalink / raw)
To: Kevin Curtis
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, Dermot Smith
In-Reply-To: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA4@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:12 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 5 of 7
Please run your patches through checkpatch
total: 4178 errors, 6155 warnings, 9910 lines checked
^ permalink raw reply
* Re: [PATCH 004/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Ben Hutchings @ 2013-09-18 15:39 UTC (permalink / raw)
To: Kevin Curtis
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, Dermot Smith
In-Reply-To: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA3@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:12 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 4 of 7
> Note that this patch must be applied with patch 5 (farsync_driver_patch)
Then don't make them separate patches.
> Update the existing farsync.h file for the new features of the farsync and
> flex drivers.
>
> Signed-off-by: Kevin Curtis <kevin.curtis@farsite.com>
>
> ---
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/drivers/net/wan/farsync.h linux-3.10.1_new/drivers/net/wan/farsync.h
> --- linux-3.10.1/drivers/net/wan/farsync.h 2013-07-13 19:42:41.000000000 +0100
> +++ linux-3.10.1_new/drivers/net/wan/farsync.h 2013-09-16 16:30:06.483104880 +0100
[...]
> +#ifdef __x86_64__
> +#define FST_PLATFORM "64bit"
> +#else
> +#define FST_PLATFORM "32bit"
> +#endif
Really, there are a lot more 64-bit architectures than that...
> +#define FST_ADDITIONAL FST_BUILD_NO
> +
> +#define FST_INCLUDES_CHAR
> +
> +struct fst_device_stats {
> + unsigned long rx_packets; /* total packets received */
> + unsigned long tx_packets; /* total packets transmitted */
> + unsigned long rx_bytes; /* total bytes received */
> + unsigned long tx_bytes; /* total bytes transmitted */
> + unsigned long rx_errors; /* bad packets received */
> + unsigned long tx_errors; /* packet transmit problems */
> + unsigned long rx_dropped; /* no space in linux buffers */
> + unsigned long tx_dropped; /* no space available in linux */
> + unsigned long multicast; /* multicast packets received */
> + unsigned long collisions;
> +
> + /* detailed rx_errors: */
> + unsigned long rx_length_errors;
> + unsigned long rx_over_errors; /* receiver ring buff overflow */
> + unsigned long rx_crc_errors; /* recved pkt with crc error */
> + unsigned long rx_frame_errors; /* recv'd frame alignment error */
> + unsigned long rx_fifo_errors; /* recv'r fifo overrun */
> + unsigned long rx_missed_errors; /* receiver missed packet */
> +
> + /* detailed tx_errors */
> + unsigned long tx_aborted_errors;
> + unsigned long tx_carrier_errors;
> + unsigned long tx_fifo_errors;
> + unsigned long tx_heartbeat_errors;
> + unsigned long tx_underrun_errors;
> +
> + /* for cslip etc */
> + unsigned long rx_compressed;
> + unsigned long tx_compressed;
> +};
Why do you need your own copy of struct net_device_stats?
[...]
> /* Ioctl call command values
> + *
> + * The first three private ioctls are used by the sync-PPP module,
> + * allowing a little room for expansion we start our numbering at 10.
> */
> -#define FSTWRITE (SIOCDEVPRIVATE+10)
> -#define FSTCPURESET (SIOCDEVPRIVATE+11)
> -#define FSTCPURELEASE (SIOCDEVPRIVATE+12)
> -#define FSTGETCONF (SIOCDEVPRIVATE+13)
> -#define FSTSETCONF (SIOCDEVPRIVATE+14)
> -
> +#define FSTWRITE (SIOCDEVPRIVATE+4)
> +#define FSTCPURESET (SIOCDEVPRIVATE+5)
> +#define FSTCPURELEASE (SIOCDEVPRIVATE+6)
> +#define FSTGETCONF (SIOCDEVPRIVATE+7)
> +#define FSTSETCONF (SIOCDEVPRIVATE+8)
> +#define FSTSNOTIFY (SIOCDEVPRIVATE+9)
> +#define FSTGSTATE (SIOCDEVPRIVATE+10)
> +#define FSTSYSREQ (SIOCDEVPRIVATE+11)
> +#define FSTGETSHELL (SIOCDEVPRIVATE+12)
> +#define FSTSETMON (SIOCDEVPRIVATE+13)
> +#define FSTSETPORT (SIOCDEVPRIVATE+14)
> +#define FSTCMD (SIOCDEVPRIVATE+15)
[...]
No, you must never renumber ioctls once they're used in production.
It's hard to know what other changes you're making, because of all the
whitespace fixes. It would be clearer if you fixed up the whitespace in
the existing header as a preparatory patch.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 003/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Ben Hutchings @ 2013-09-18 15:33 UTC (permalink / raw)
To: Kevin Curtis
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, Dermot Smith
In-Reply-To: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA2@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:12 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 3 of 7
>
> Introduce a new include file required by the fsflex driver.
>
> Signed-off-by: Kevin Curtis <kevin.curtis@farsite.com>
>
> ---
>
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/drivers/net/wan/fscmn.h linux-3.10.1_new/drivers/net/wan/fscmn.h
> --- linux-3.10.1/drivers/net/wan/fscmn.h 1970-01-01 01:00:00.000000000 +0100
> +++ linux-3.10.1_new/drivers/net/wan/fscmn.h 2013-09-16 16:30:06.779104868 +0100
[...]
> +#ifdef UINT32
> +#define u32 UINT32
> +#define u16 UINT16
> +#define u8 UCHAR
> +#endif
> +
> +#ifdef USSTYPES_H
> +#define u32 U32
> +#define u16 U16
> +#define u8 U8
> +#endif
[...]
This sort of cruft doesn't belong in an in-tree driver. unifdef may be
useful for removing it when copying in out-of-tree 'portable' code.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 003/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Joe Perches @ 2013-09-18 15:33 UTC (permalink / raw)
To: Kevin Curtis
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, Dermot Smith
In-Reply-To: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA2@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:12 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/drivers/net/wan/fscmn.h linux-3.10.1_new/drivers/net/wan/fscmn.h
[]
> +#ifdef UINT32
> +#define u32 UINT32
> +#define u16 UINT16
> +#define u8 UCHAR
> +#endif
> +
> +#ifdef USSTYPES_H
> +#define u32 U32
> +#define u16 U16
> +#define u8 U8
> +#endif
Unpretty.
Why aren't the typedefs from
include/asm-generic good enough?
> +typedef struct _FSCMN_TXRX_PREAMBLE {
[]
> +} FSCMN_TXRX_PREAMBLE, *PFSCMN_TXRX_PREAMBLE;
More typedefs
^ permalink raw reply
* Re: [PATCH 002/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Ben Hutchings @ 2013-09-18 15:30 UTC (permalink / raw)
To: Kevin Curtis
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, Dermot Smith
In-Reply-To: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA1@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:11 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 2 of 7
>
> Introduce a new include file required by the fsflex driver.
>
> Signed-off-by: Kevin Curtis <kevin.curtis@farsite.com>
>
> ---
>
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/drivers/net/wan/uss_cmn.h linux-3.10.1_new/drivers/net/wan/uss_cmn.h
> --- linux-3.10.1/drivers/net/wan/uss_cmn.h 1970-01-01 01:00:00.000000000 +0100
> +++ linux-3.10.1_new/drivers/net/wan/uss_cmn.h 2013-08-30 11:30:36.766167447 +0100
> @@ -0,0 +1,492 @@
> +/*
> + * FarSync driver for Linux
> + *
> + * Copyright (C) 2001-2013 FarSite Communications Ltd.
> + * www.farsite.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * File : uss_cmn.h
> + *
> + * Description : FarSync Flex common header file, shared between oncard code
> + * and Windows and Linux drivers.
> + *
> + * NOTE: all parameters are in Little Endian format
Then they should be declared as __le32 or similar.
[...]
> +typedef enum _USS_COMMANDS
[...]
> +} USS_COMMANDS;
Adding typedefs for user-defined types is deprecated. And please use
lower-case type names.
[...]
> +/*****************************************************************************/
> +/* InterfaceRecord definition */
> +/* */
> +/* For use with: */
> +/* */
> +/* IoctlCodeReadInterfaceRecord, */
> +/* IoctlCodeFarSyncReadInterfaceRecord */
> +/* */
> +/*****************************************************************************/
> +typedef struct _INTERFACE_RECORD {
> + int rx_frame_count; /* incremented after each frame rx'd */
> + int tx_max_fr_size_now; /* max available frame size av. now */
> + /* (changes after each Tx DevIoctl */
> + /* to DD or after Tx completed) */
> + int status_count; /* How many status events have been */
> + /* triggered. */
> + u8 v24_in; /* Last 'getv24 i/p' value got */
> + u8 v24_out; /* Last 'setv24 o/p' value set */
[...]
I think explicit padding is needed between v24_out and status_array.
Not all architectures require 32-bit words to be 32-bit aligned.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [RFC] b44: use phylib
From: Florian Fainelli @ 2013-09-18 15:29 UTC (permalink / raw)
To: Joe Perches; +Cc: Hauke Mehrtens, zambrano, netdev
In-Reply-To: <1379517545.1787.20.camel@joe-AO722>
2013/9/18 Joe Perches <joe@perches.com>:
> On Sat, 2013-08-24 at 00:56 +0200, Hauke Mehrtens wrote:
>> This splits the driver into the mac and a phy driver. On routers where
>> this driver is used we have a switch which implements a phy and can be
>> controlled by a phy driver.
>
> One more trivial note:
>
>> diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
> []
>> +static void b44_adjust_link(struct net_device *dev)
>> +{
> []
>> + if (status_changed) {
>> + pr_info("%s: link %s", dev->name, phydev->link ?
>> + "UP" : "DOWN");
>> + if (phydev->link)
>> + pr_cont(" - %d/%s", phydev->speed,
>> + phydev->duplex == DUPLEX_FULL ? "full" : "half");
>> + pr_cont("\n");
>> + }
>
> This bit would be better as:
>
> if (status_change) {
> if (phydev->link)
> netdev_info(dev, "link UP - %d/%s\n",
> phydev->speed,
> phydev->duplex == DUPLEX_FULL
> ? "full" : "half");
> else
> netdev_info(dev, "link DOWN\n");
> }
There is a helper function provided by phylib for this: phy_print_status().
--
Florian
^ permalink raw reply
* Re: [PATCH 002/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Joe Perches @ 2013-09-18 15:25 UTC (permalink / raw)
To: Kevin Curtis
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, Dermot Smith
In-Reply-To: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA1@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:11 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
[]
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/drivers/net/wan/uss_cmn.h linux-3.10.1_new/drivers/net/wan/uss_cmn.h
> +typedef enum _USS_COMMANDS {
I think this code would be more linux style
without all the typedefs
> +typedef struct {
[]
> +} FLEX_SERCONF, *PFLEX_SERCONF;
> +typedef struct {
[]
> +} FLEX_CARD_INFO, *PFLEX_CARD_INFO; /* sizeof(FLEX_CARD_INFO) = 128 */
> +typedef struct {
[]
> +} FLEX_USB_STATS, *PFLEX_USB_STATS;
> +typedef struct _INTERFACE_RECORD {
[]
> +} IR, *PIR;
> +typedef struct _INTERFACE_RECORD_EX {
[]
> +} IREX, *PIREX;
^ permalink raw reply
* Re: [PATCH 001/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Ben Hutchings @ 2013-09-18 15:23 UTC (permalink / raw)
To: Kevin Curtis
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, Dermot Smith
In-Reply-To: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA0@SOLO.hq.farsitecommunications.com>
Don't use the same subject line for all your patches. This will be the
summary of the change in git and it needs to say what the individual
change does.
On Wed, 2013-09-18 at 11:11 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 1 of 7
>
> Add new FarSite PCI ID's to the pci_ids.h file
This is unnecessary unless they will be used in multiple drivers.
Ben.
> Signed-off-by: Kevin Curtis <kevin.curtis@farsite.com>
>
> ---
>
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/include/linux/pci_ids.h linux-3.10.1_new/include/linux/pci_ids.h
> --- linux-3.10.1/include/linux/pci_ids.h 2013-07-13 19:42:41.000000000 +0100
> +++ linux-3.10.1_new/include/linux/pci_ids.h 2013-07-26 11:18:39.821065185 +0100
> @@ -2284,6 +2284,14 @@
> #define PCI_DEVICE_ID_FARSITE_T4U 0x0640
> #define PCI_DEVICE_ID_FARSITE_TE1 0x1610
> #define PCI_DEVICE_ID_FARSITE_TE1C 0x1612
> +#define PCI_DEVICE_ID_FARSITE_DSL_S1 0x2610
> +#define PCI_DEVICE_ID_FARSITE_T4E 0x3640
> +#define PCI_DEVICE_ID_FARSITE_T4UE 0x4640
> +#define PCI_DEVICE_ID_FARSITE_T2UE 0x4620
> +#define PCI_DEVICE_ID_FARSITE_T2U_PMC 0x6620
> +#define PCI_DEVICE_ID_FARSITE_T2Ee 0x5621
> +#define PCI_DEVICE_ID_FARSITE_T4Ee 0x5641
> +
>
> #define PCI_VENDOR_ID_ARIMA 0x161f
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [REGRESSION][BISECTED] skge: add dma_mapping check
From: Ben Hutchings @ 2013-09-18 15:22 UTC (permalink / raw)
To: Stephen Hemminger, netdev; +Cc: Mirko Lindner, linux-kernel, Igor Gnatenko
In-Reply-To: <1379494844.2294.12.camel@ThinkPad-X230.localdomain>
On Wed, 2013-09-18 at 13:00 +0400, Igor Gnatenko wrote:
> Since 136d8f377e1575463b47840bc5f1b22d94bf8f63 commit we have kernel
> panic on:
[...]
At a first glance, it looks like this bit is wrong:
> @@ -3058,13 +3090,17 @@ static struct sk_buff *skge_rx_get(struct net_device *dev,
> if (!nskb)
> goto resubmit;
>
> + if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
> + dev_kfree_skb(nskb);
> + goto resubmit;
> + }
> +
> pci_unmap_single(skge->hw->pdev,
> dma_unmap_addr(e, mapaddr),
> dma_unmap_len(e, maplen),
> PCI_DMA_FROMDEVICE);
> skb = e->skb;
> prefetch(skb->data);
> - skge_rx_setup(skge, e, nskb, skge->rx_buf_size);
> }
>
> skb_put(skb, len);
That pci_unmap_single() appears to unmap the *new*, rather than old, DMA
mapping. I think you need to copy out the old DMA address and length
before doing skge_rx_setup().
Try it with an IOMMU in strict mode (intel_iommu=on,strict or
amd_iommu=fullflush).
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [RFC] b44: use phylib
From: Joe Perches @ 2013-09-18 15:19 UTC (permalink / raw)
To: Hauke Mehrtens; +Cc: zambrano, netdev, Florian Fainelli
In-Reply-To: <1377298608-18016-1-git-send-email-hauke@hauke-m.de>
On Sat, 2013-08-24 at 00:56 +0200, Hauke Mehrtens wrote:
> This splits the driver into the mac and a phy driver. On routers where
> this driver is used we have a switch which implements a phy and can be
> controlled by a phy driver.
One more trivial note:
> diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
[]
> +static void b44_adjust_link(struct net_device *dev)
> +{
[]
> + if (status_changed) {
> + pr_info("%s: link %s", dev->name, phydev->link ?
> + "UP" : "DOWN");
> + if (phydev->link)
> + pr_cont(" - %d/%s", phydev->speed,
> + phydev->duplex == DUPLEX_FULL ? "full" : "half");
> + pr_cont("\n");
> + }
This bit would be better as:
if (status_change) {
if (phydev->link)
netdev_info(dev, "link UP - %d/%s\n",
phydev->speed,
phydev->duplex == DUPLEX_FULL
? "full" : "half");
else
netdev_info(dev, "link DOWN\n");
}
so there's no possible interleaving by other
thread messages.
^ permalink raw reply
* Re: [PATCH] USBNET: fix handling padding packet
From: Ming Lei @ 2013-09-18 15:09 UTC (permalink / raw)
To: Bjørn Mork
Cc: David S. Miller, Greg Kroah-Hartman, Oliver Neukum,
Network Development, linux-usb, Oliver Neukum
In-Reply-To: <878uyu6xjm.fsf@nemi.mork.no>
On Wed, Sep 18, 2013 at 9:59 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
>> if the usb host controller is capable of building packet from
>> discontinuous buffers, but missed handling padding packet when
>> building DMA SG.
>>
>> This patch attachs the pre-allocated padding packet at the
>> end of the sg list, so padding packet can be sent to device
>> if drivers require that.
>>
>> Reported-by: David Laight <David.Laight@aculab.com>
>> Cc: Oliver Neukum <oliver@neukum.org>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>> drivers/net/usb/usbnet.c | 27 +++++++++++++++++++++------
>> include/linux/usb/usbnet.h | 1 +
>> 2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 7b331e6..4a9bed3 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb, struct urb *urb)
>> if (num_sgs == 1)
>> return 0;
>>
>> - urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
>> + /* reserve one for zero packet */
>> + urb->sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
>> + GFP_ATOMIC);
>> if (!urb->sg)
>> return -ENOMEM;
>>
>> @@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
>> if (build_dma_sg(skb, urb) < 0)
>> goto drop;
>> }
>> - entry->length = length = urb->transfer_buffer_length;
>> + length = urb->transfer_buffer_length;
>>
>> /* don't assume the hardware handles USB_ZERO_PACKET
>> * NOTE: strictly conforming cdc-ether devices should expect
>> @@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
>> if (length % dev->maxpacket == 0) {
>> if (!(info->flags & FLAG_SEND_ZLP)) {
>> if (!(info->flags & FLAG_MULTI_PACKET)) {
>> - urb->transfer_buffer_length++;
>> - if (skb_tailroom(skb)) {
>> + length++;
>> + if (skb_tailroom(skb) && !dev->can_dma_sg) {
>> skb->data[skb->len] = 0;
>> __skb_put(skb, 1);
>> - }
>> + } else if (dev->can_dma_sg)
>> + sg_set_buf(&urb->sg[urb->num_sgs++],
>> + dev->padding_pkt, 1);
>> }
>> } else
>> urb->transfer_flags |= URB_ZERO_PACKET;
>> }
>> + entry->length = urb->transfer_buffer_length = length;
>>
>> spin_lock_irqsave(&dev->txq.lock, flags);
>> retval = usb_autopm_get_interface_async(dev->intf);
>> @@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>>
>> usb_kill_urb(dev->interrupt);
>> usb_free_urb(dev->interrupt);
>> + kfree(dev->padding_pkt);
>>
>> free_netdev(net);
>> }
>> @@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>> /* initialize max rx_qlen and tx_qlen */
>> usbnet_update_max_qlen(dev);
>>
>> + if (dev->can_dma_sg && !(info->flags & FLAG_SEND_ZLP) &&
>> + !(info->flags & FLAG_MULTI_PACKET)) {
>> + dev->padding_pkt = kzalloc(1, GFP_KERNEL);
>> + if (!dev->padding_pkt)
>> + goto out4;
>> + }
>> +
>> status = register_netdev (net);
>> if (status)
>> - goto out4;
>> + goto out5;
>> netif_info(dev, probe, dev->net,
>> "register '%s' at usb-%s-%s, %s, %pM\n",
>> udev->dev.driver->name,
>> @@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>>
>> return 0;
>>
>> +out5:
>> + kfree(dev->padding_pkt);
>> out4:
>> usb_free_urb(dev->interrupt);
>> out3:
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>> index 9cb2fe8..e303eef 100644
>> --- a/include/linux/usb/usbnet.h
>> +++ b/include/linux/usb/usbnet.h
>> @@ -42,6 +42,7 @@ struct usbnet {
>> struct usb_host_endpoint *status;
>> unsigned maxpacket;
>> struct timer_list delay;
>> + const char *padding_pkt;
>>
>> /* protocol/interface state */
>> struct net_device *net;
>
>
> The code handling the frame padding was already unecessarily complex
> IMHO, and this does not improve it...
>
> Are you really sure that the one driver/device using this really need
> the padding byte? If you could just make FLAG_SEND_ZLP part of the
> condition for enabling can_dma_sg, then all this extra complexity would
> be unnecessary. As the comment in front of the padding code says:
At least for the effected driver of ax88179, the padding packet is needed
since the driver does set the padding flag in the header, see
ax88179_tx_fixup().
>
> "strictly conforming cdc-ether devices should expect the ZLP here"
>
> There shouldn't be any problems requiring this conformance as a
> precondition for allowing SG. The requirements are already strict.
There is no reason to forbid DMA SG for one driver which requires
padding, right?
>
>
> I also believe it would be nice to move the initialisation of can_dma_sg
> away from the minidriver and into usbnet_probe. It's confusing that
> this field is used "uninitialized" (well, defaulting to zero) in all but
> one minidriver. It would much nicer if the logic was more like
Looks the above and below isn't related with the patch closely, and
patch is welcome to simplify code.
>
> usbnet_probe:
> if (...)
> dev->can_dma_sg = 1;
>
> minidriver_bind:
> if (dev->can_dma_sg) {
> dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
> dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
> }
>
> usbnet_start_xmit:
> if (skb_shinfo(skb)->nr_frags) {
> ...
> }
>
> This would create a natural flow of events, where probing sets the
> capability, minidriver enables features based on capability, and xmit
> just does what it has to do based on the skb it was given.
>
> Or maybe even better: factor out common parts of usbnet_start_xmit and
> create two versions of it - one using SG and one linear. The per packet
> testing seems unnecessary expensive and complex given that the choice is
> made during device initialization anyway. You could easily replace the
> whole can_dma_sg stuff with a simple
>
> if (...)
> net->netdev_ops = &usbnet_sg_netdev_ops;
> else
> net->netdev_ops = &usbnet_netdev_ops;
>
>
> in usbnet_probe. But I guess the same can be said about the info->flags
> testing in usbnet_start_xmit...
>
> Just a few random thougths. I don't really feel strongly about any of
> this, except that I would prefer usbnet becoming *more* readable instead
> of less...
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [RFC] b44: use phylib
From: Hauke Mehrtens @ 2013-09-18 15:03 UTC (permalink / raw)
To: zambrano; +Cc: netdev, Florian Fainelli
In-Reply-To: <1377298608-18016-1-git-send-email-hauke@hauke-m.de>
On 08/24/2013 12:56 AM, Hauke Mehrtens wrote:
> This splits the driver into the mac and a phy driver. On routers where
> this driver is used we have a switch which implements a phy and can be
> controlled by a phy driver.
>
> I have just tested this on my router with a switch as phy and not on normal desktop PC.
>
> This is based on a patch by Florian Fainelli <florian@openwrt.org>
Hi Gary Zambrano,
Could you please test this patch or could someone else please test this
patch with a b44 chip in a desktop or laptop. I have just tested this
patch on some routers where a BCM53XX switch is used as PHY, but do not
own a device where b44 is used as a normal NIC.
Hauke
^ permalink raw reply
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs
From: Grant Likely @ 2013-09-18 15:00 UTC (permalink / raw)
To: Florian Fainelli
Cc: Thomas Petazzoni, David S. Miller, netdev,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Lior Amsalem,
Mark Rutland, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia,
Gregory Clement,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <CAGVrzcbVTenhVC4tQznJFqVpO08ruxLyy1ZiLmw6Bu1=3zbGZg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, 18 Sep 2013 10:21:11 +0100, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello,
>
> 2013/9/18 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> > On Fri, 6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> Some Ethernet MACs have a "fixed link", and are not connected to a
> >> normal MDIO-managed PHY device. For those situations, a Device Tree
> >> binding allows to describe a "fixed link" using a special PHY node.
> >>
> >> This patch adds:
> >>
> >> * A documentation for the fixed PHY Device Tree binding.
> >>
> >> * An of_phy_is_fixed_link() function that an Ethernet driver can call
> >> on its PHY phandle to find out whether it's a fixed link PHY or
> >> not. It should typically be used to know if
> >> of_phy_register_fixed_link() should be called.
> >>
> >> * An of_phy_register_fixed_link() function that instantiates the
> >> fixed PHY into the PHY subsystem, so that when the driver calls
> >> of_phy_connect(), the PHY device associated to the OF node will be
> >> found.
> >>
> >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >
> > Hi Thomas,
> >
> > The implemenation in this series looks like it is in good shape, so I'll
> > restrict my comments to be binding...
> >
> >> ---
> >> .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++
> >> drivers/of/of_mdio.c | 24 +++++++++++++++
> >> include/linux/of_mdio.h | 15 ++++++++++
> >> 3 files changed, 73 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
> >> new file mode 100644
> >> index 0000000..9f2a1a50
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> >> @@ -0,0 +1,34 @@
> >> +Fixed link Device Tree binding
> >> +------------------------------
> >> +
> >> +Some Ethernet MACs have a "fixed link", and are not connected to a
> >> +normal MDIO-managed PHY device. For those situations, a Device Tree
> >> +binding allows to describe a "fixed link".
> >> +
> >> +Such a fixed link situation is described by creating a PHY node as a
> >> +sub-node of an Ethernet device, with the following properties:
> >> +
> >> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a
> >> + fixed link PHY.
> >> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted
> >> + values are 10, 100 and 1000
> >> +* 'full-duplex' (boolean, optional), to indicate that full duplex is
> >> + used. When absent, half duplex is assumed.
> >> +* 'pause' (boolean, optional), to indicate that pause should be
> >> + enabled.
> >> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should
> >> + be enabled.
> >
> > I understand what you're trying to do here, but it causes a troublesome
> > leakage of implementation detail into the binding, making the whole
> > thing look very odd. This binding tries to make a fixed link look
> > exactly like a real PHY even to the point of including a phandle to the
> > phy. But having a phandle to a node which is *always* a direct child of
> > the MAC node is redundant and a rather looney. Yes, doing it that way
> > makes it easy for of_phy_find_device() to be transparent for fixed link,
> > but that should *not* drive bindings, especially when that makes the
> > binding really rather weird.
>
> This is not exactly true in the sense that the "new" binding just
> re-shuffles the properties representation into something that is
> clearer and more extendible but there is not much difference in the
> semantics.
That's not my point in the above paragraph. My point is a binding that
consists of a phandle to a node that is always a direct child is goofy
and wrong.
> > Second, this new binding doesn't provide anything over and above the
> > existing fixed-link binding. It may not be pretty, but it is
> > estabilshed.
>
> In fact it does, the old one is obscure and not easily extendable
> because we rely on an integer array to represent the various
> properties, so at least this new one makes it easy to extend the
> binding to support a possibly new fixed-link property. Being able to
> deprecate a fundamentaly badly designed binding should still be a
> prerogative, software is flexible and can deal with both with little
> cost.
I understand that, but consistency is also important. I don't see a
proposal for a new feature for the binding in this patch. Without a
really compelling reason to change a binding that works (even if it is
opaque) I cannot agree to changing it.
> > That said, I do agree that the current Linux implementation is not good
> > because it cannot handle a fixed-link property transparently. That's a
> > deficiency in the Linux implementation and it should be fixed.
> > of_phy_connect() currently requires the phy phandle to be passed in.
> > Part of the reason it was done this way is that some drivers connect to
> > multiple 'phys'. A soulition could be to make the phy handle optional.
> > If it is empty then go looking for either a phy-device or fixed-link
> > property. Otherwise use the provided node.
>
> I do not quite follow you on this one, and I fear we might be leaking
> some Linux probing heuristic into Device Tree bindings by implicitely
> saying "not including a PHY phandle means connecting to a fixed-PHY
> link." This would also dramatically change the current behavior for
> most drivers where they might refuse probing if no corresponding PHY
> device node is present and they are not designed to connect to a fixed
> PHY one.
Drivers we can change and fix. There aren't very may call sites
affected so I'm not overly worried about it. Also, I was making a
suggestion on how to fix it, but a suggestion is not a fully formed
patch. The issue you raise would of course need to be addressed.
g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC] b44: use phylib
From: Hauke Mehrtens @ 2013-09-18 15:00 UTC (permalink / raw)
To: Joe Perches; +Cc: zambrano, netdev, Florian Fainelli
In-Reply-To: <1377299409.2816.19.camel@joe-AO722>
On 08/24/2013 01:10 AM, Joe Perches wrote:
> On Sat, 2013-08-24 at 00:56 +0200, Hauke Mehrtens wrote:
>> This splits the driver into the mac and a phy driver. On routers where
>> this driver is used we have a switch which implements a phy and can be
>> controlled by a phy driver.
>
> trivial comments only...
I will improve that
>> diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
> []
>> +static void b44_adjust_link(struct net_device *dev)
>> +{
>> + struct b44 *bp = netdev_priv(dev);
>> + struct phy_device *phydev = bp->phydev;
>> + int status_changed = 0;
>
> bool?
Yes, next time.
>> +static int b44_mii_probe(struct net_device *dev)
>> +{
> []
>> + if (IS_ERR(phydev)) {
>> + netdev_err(dev, "could not attach PHY: %s", phy_id);
>
> missing newline?
Yes, next time.
^ permalink raw reply
* Re: [PATCH] USBNET: fix handling padding packet
From: Bjørn Mork @ 2013-09-18 13:59 UTC (permalink / raw)
To: Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman, Oliver Neukum, netdev,
linux-usb, Oliver Neukum
In-Reply-To: <1379409002-7698-1-git-send-email-ming.lei@canonical.com>
Ming Lei <ming.lei@canonical.com> writes:
> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
> if the usb host controller is capable of building packet from
> discontinuous buffers, but missed handling padding packet when
> building DMA SG.
>
> This patch attachs the pre-allocated padding packet at the
> end of the sg list, so padding packet can be sent to device
> if drivers require that.
>
> Reported-by: David Laight <David.Laight@aculab.com>
> Cc: Oliver Neukum <oliver@neukum.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> drivers/net/usb/usbnet.c | 27 +++++++++++++++++++++------
> include/linux/usb/usbnet.h | 1 +
> 2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 7b331e6..4a9bed3 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb, struct urb *urb)
> if (num_sgs == 1)
> return 0;
>
> - urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
> + /* reserve one for zero packet */
> + urb->sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
> + GFP_ATOMIC);
> if (!urb->sg)
> return -ENOMEM;
>
> @@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
> if (build_dma_sg(skb, urb) < 0)
> goto drop;
> }
> - entry->length = length = urb->transfer_buffer_length;
> + length = urb->transfer_buffer_length;
>
> /* don't assume the hardware handles USB_ZERO_PACKET
> * NOTE: strictly conforming cdc-ether devices should expect
> @@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
> if (length % dev->maxpacket == 0) {
> if (!(info->flags & FLAG_SEND_ZLP)) {
> if (!(info->flags & FLAG_MULTI_PACKET)) {
> - urb->transfer_buffer_length++;
> - if (skb_tailroom(skb)) {
> + length++;
> + if (skb_tailroom(skb) && !dev->can_dma_sg) {
> skb->data[skb->len] = 0;
> __skb_put(skb, 1);
> - }
> + } else if (dev->can_dma_sg)
> + sg_set_buf(&urb->sg[urb->num_sgs++],
> + dev->padding_pkt, 1);
> }
> } else
> urb->transfer_flags |= URB_ZERO_PACKET;
> }
> + entry->length = urb->transfer_buffer_length = length;
>
> spin_lock_irqsave(&dev->txq.lock, flags);
> retval = usb_autopm_get_interface_async(dev->intf);
> @@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>
> usb_kill_urb(dev->interrupt);
> usb_free_urb(dev->interrupt);
> + kfree(dev->padding_pkt);
>
> free_netdev(net);
> }
> @@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
> /* initialize max rx_qlen and tx_qlen */
> usbnet_update_max_qlen(dev);
>
> + if (dev->can_dma_sg && !(info->flags & FLAG_SEND_ZLP) &&
> + !(info->flags & FLAG_MULTI_PACKET)) {
> + dev->padding_pkt = kzalloc(1, GFP_KERNEL);
> + if (!dev->padding_pkt)
> + goto out4;
> + }
> +
> status = register_netdev (net);
> if (status)
> - goto out4;
> + goto out5;
> netif_info(dev, probe, dev->net,
> "register '%s' at usb-%s-%s, %s, %pM\n",
> udev->dev.driver->name,
> @@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>
> return 0;
>
> +out5:
> + kfree(dev->padding_pkt);
> out4:
> usb_free_urb(dev->interrupt);
> out3:
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index 9cb2fe8..e303eef 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -42,6 +42,7 @@ struct usbnet {
> struct usb_host_endpoint *status;
> unsigned maxpacket;
> struct timer_list delay;
> + const char *padding_pkt;
>
> /* protocol/interface state */
> struct net_device *net;
The code handling the frame padding was already unecessarily complex
IMHO, and this does not improve it...
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
"strictly conforming cdc-ether devices should expect the ZLP here"
There shouldn't be any problems requiring this conformance as a
precondition for allowing SG. The requirements are already strict.
I also believe it would be nice to move the initialisation of can_dma_sg
away from the minidriver and into usbnet_probe. It's confusing that
this field is used "uninitialized" (well, defaulting to zero) in all but
one minidriver. It would much nicer if the logic was more like
usbnet_probe:
if (...)
dev->can_dma_sg = 1;
minidriver_bind:
if (dev->can_dma_sg) {
dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
}
usbnet_start_xmit:
if (skb_shinfo(skb)->nr_frags) {
...
}
This would create a natural flow of events, where probing sets the
capability, minidriver enables features based on capability, and xmit
just does what it has to do based on the skb it was given.
Or maybe even better: factor out common parts of usbnet_start_xmit and
create two versions of it - one using SG and one linear. The per packet
testing seems unnecessary expensive and complex given that the choice is
made during device initialization anyway. You could easily replace the
whole can_dma_sg stuff with a simple
if (...)
net->netdev_ops = &usbnet_sg_netdev_ops;
else
net->netdev_ops = &usbnet_netdev_ops;
in usbnet_probe. But I guess the same can be said about the info->flags
testing in usbnet_start_xmit...
Just a few random thougths. I don't really feel strongly about any of
this, except that I would prefer usbnet becoming *more* readable instead
of less...
Bjørn
^ permalink raw reply
* Re: [PATCH] USBNET: fix handling padding packet
From: Oliver Neukum @ 2013-09-18 13:46 UTC (permalink / raw)
To: Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1379409002-7698-1-git-send-email-ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
On Tue, 2013-09-17 at 17:10 +0800, Ming Lei wrote:
> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
> if the usb host controller is capable of building packet from
> discontinuous buffers, but missed handling padding packet when
> building DMA SG.
>
> This patch attachs the pre-allocated padding packet at the
> end of the sg list, so padding packet can be sent to device
> if drivers require that.
>
> Reported-by: David Laight <David.Laight-JxhZ9S5GRejQT0dZR+AlfA@public.gmane.org>
> Cc: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Aida Mynzhasova @ 2013-09-18 13:21 UTC (permalink / raw)
To: richardcochran; +Cc: netdev
IEEE 1588 timer reference clock source is determined through hard-coded
value in gianfar_ptp driver by default. This patch allows to select ptp
clock source by means of device tree file node.
For instance:
fsl,cksel = <0>;
for using external (TSEC_TMR_CLK input) high precision timer
reference clock.
Other acceptable values:
<1> : eTSEC system clock
<2> : eTSEC1 transmit clock
<3> : RTC clock input
Signed-off-by: Aida Mynzhasova <aida.mynzhasova@skitlab.ru>
---
drivers/net/ethernet/freescale/gianfar_ptp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/gianfar_ptp.c b/drivers/net/ethernet/freescale/gianfar_ptp.c
index 098f133..e006a09 100644
--- a/drivers/net/ethernet/freescale/gianfar_ptp.c
+++ b/drivers/net/ethernet/freescale/gianfar_ptp.c
@@ -452,7 +452,9 @@ static int gianfar_ptp_probe(struct platform_device *dev)
err = -ENODEV;
etsects->caps = ptp_gianfar_caps;
- etsects->cksel = DEFAULT_CKSEL;
+
+ if (get_of_u32(node, "fsl,cksel", &etsects->cksel))
+ etsects->cksel = DEFAULT_CKSEL;
if (get_of_u32(node, "fsl,tclk-period", &etsects->tclk_period) ||
get_of_u32(node, "fsl,tmr-prsc", &etsects->tmr_prsc) ||
--
1.8.1.2
^ permalink raw reply related
* [PATCH] net: tsi108: Prevent compiler warning
From: Thierry Reding @ 2013-09-18 12:49 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel
The dump_eth_one() function is only used if DEBUG is enabled, so protect
it by a corresponding #ifdef DEBUG block.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
drivers/net/ethernet/tundra/tsi108_eth.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/tundra/tsi108_eth.c b/drivers/net/ethernet/tundra/tsi108_eth.c
index c4dbf98..a452d93 100644
--- a/drivers/net/ethernet/tundra/tsi108_eth.c
+++ b/drivers/net/ethernet/tundra/tsi108_eth.c
@@ -168,6 +168,7 @@ static struct platform_driver tsi_eth_driver = {
static void tsi108_timed_checker(unsigned long dev_ptr);
+#ifdef DEBUG
static void dump_eth_one(struct net_device *dev)
{
struct tsi108_prv_data *data = netdev_priv(dev);
@@ -192,6 +193,7 @@ static void dump_eth_one(struct net_device *dev)
TSI_READ(TSI108_EC_RXESTAT),
TSI_READ(TSI108_EC_RXERR), data->rxpending);
}
+#endif
/* Synchronization is needed between the thread and up/down events.
* Note that the PHY is accessed through the same registers for both
--
1.8.4
^ permalink raw reply related
* [PATCH ] ip6tnl: do route updating for redirect in ip6_tnl_err()
From: Duan Jiong @ 2013-09-18 12:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, hannes
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
After the ip6_tnl_err() is called, the rel_msg is assigned
to 0, and the ip4ip6_err()will return directly, the instruction
below will not be executed.
In rfc2473, we can know that the tunnel ICMP redirect
message should not be reported to the source of the
original packet, so we can handle it in ip4ip6_err().
Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
net/ipv6/ip6_tunnel.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 2d8f482..35c4b70 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -529,6 +529,9 @@ ip6_tnl_err(struct sk_buff *skb, __u8 ipproto, struct inet6_skb_parm *opt,
rel_msg = 1;
}
break;
+ case NDISC_REDIRECT:
+ ip6_redirect(skb, net, 0, 0);
+ break;
}
*type = rel_type;
@@ -576,9 +579,6 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
rel_type = ICMP_DEST_UNREACH;
rel_code = ICMP_FRAG_NEEDED;
break;
- case NDISC_REDIRECT:
- rel_type = ICMP_REDIRECT;
- rel_code = ICMP_REDIR_HOST;
default:
return 0;
}
@@ -637,8 +637,6 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
skb_dst(skb2)->ops->update_pmtu(skb_dst(skb2), NULL, skb2, rel_info);
}
- if (rel_type == ICMP_REDIRECT)
- skb_dst(skb2)->ops->redirect(skb_dst(skb2), NULL, skb2);
icmp_send(skb2, rel_type, rel_code, htonl(rel_info));
--
1.8.3.1
^ permalink raw reply related
* [PATCH ] ipv6: handle the update of the NDISC_REDIRECT error code in icmpv6_err_convert
From: Duan Jiong @ 2013-09-18 12:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, hannes
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
when dealing with redirect message in udpv6_err() and
rawv6_err() the err shoud be assigned to 0, not EPROTO.
Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
net/ipv6/icmp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index eef8d94..795f348 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -977,6 +977,9 @@ int icmpv6_err_convert(u8 type, u8 code, int *err)
case ICMPV6_TIME_EXCEED:
*err = EHOSTUNREACH;
break;
+ case NDISC_REDIRECT:
+ *err = 0;
+ break;
}
return fatal;
--
1.8.3.1
^ permalink raw reply related
* [PATCH ] net:dccp: do not report ICMP redirects to user space
From: Duan Jiong @ 2013-09-18 12:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, hannes
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
DCCP shouldn't be setting sk_err on redirects as it
isn't an error condition. it should be doing exactly
what tcp is doing and leaving the error handler without
touching the socket.
Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
net/dccp/ipv6.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 9c61f9c..6cf9f77 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -135,6 +135,7 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
if (dst)
dst->ops->redirect(dst, sk, skb);
+ goto out;
}
if (type == ICMPV6_PKT_TOOBIG) {
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer
From: Duan Jiong @ 2013-09-18 11:57 UTC (permalink / raw)
To: David Miller, hannes; +Cc: netdev
In-Reply-To: <20130918041337.GD8947@order.stressinduktion.org>
于 2013年09月18日 12:13, Hannes Frederic Sowa 写道:
> On Wed, Sep 18, 2013 at 09:52:42AM +0800, Duan Jiong wrote:
>> 于 2013年09月18日 09:39, Hannes Frederic Sowa 写道:
>>> On Tue, Sep 17, 2013 at 08:29:36PM -0400, David Miller wrote:
>>>> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>> Date: Fri, 13 Sep 2013 11:03:07 +0800
>>>>
>>>>> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>>>
>>>>> Do the whole verification and route updating in ndisc
>>>>> lay and then just call into icmpv6_notify() to notify
>>>>> the upper protocols.
>>>>>
>>>>> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>>
>>>> This is completely broken, and I believe your patch set fundamentally
>>>> is too.
>>>>
>>>> We absolutely _must_ handle the redirect at the socket level when
>>>> we are able to, otherwise we cannot specify the mark properly and
>>>> the mark is an essential part of the key used to find the correct
>>>> route to work with.
>>>>
>>>> I am not applying this patch series until you deal with this
>>>> deficiency. I am not willing to consider changes which stop using the
>>>> more precise keying information available from a socket.
>>>
>>> Oh, Duan, I am very sorry for not catching this earlier. We use the
>>> sk->mark to select the proper routing table where we clone the rt6_info into.
>>> And we only get that value out of the sockets. I missed that. We should leave
>>> the redirect logic in the socket layer where it is possible.
>>>
>>> But parts of this series are still valid. We need to fix redirects for tunnels
>>> and I do think we can still simplify some code in the error handlers.
>>>
>>
>> I got it.
>
> I gave it a bit more thought:
>
> RFC 4861 8.3:
> "
> Redirect messages apply to all flows that are being sent to a given
> destination. That is, upon receipt of a Redirect for a Destination
> Address, all Destination Cache entries to that address should be
> updated to use the specified next-hop, regardless of the contents of
> the Flow Label field that appears in the Redirected Header option.
> "
>
> Especially because redirects also help in the on-link determination (same
> RFC, section 8), I changed my mind and am still in favour of updating it
> in the ndisc layer. In my opinion we just have to consider all routing
> tables and apply the update to every one which carries a valid next hop
> to the source of the redirect (under consideration of the destination).
>
> This will be important if we actually try to get linux to correctly
> implement the ipv6 subnet model (RFC 5942, Section 4 Rule 1). In that
> case we are not allowed to assume nodes on-link even if they would match
> the same prefix as a locally configured address.
>
I think this need a little time to discuss with David Miller, so i will send
the other patchs to fix redirects for tunnels and to fix the sk->sk_err
problems in udpv6_err()/rawv6_err().
Thanks,
Duan
^ permalink raw reply
* Re: [PATCHv2 net] xfrm: Guard IPsec anti replay window against replay bitmap
From: Steffen Klassert @ 2013-09-18 11:16 UTC (permalink / raw)
To: Fan Du; +Cc: davem, netdev
In-Reply-To: <1379402053-19653-1-git-send-email-fan.du@windriver.com>
On Tue, Sep 17, 2013 at 03:14:13PM +0800, Fan Du wrote:
> For legacy IPsec anti replay mechanism:
>
> bitmap in struct xfrm_replay_state could only provide a 32 bits
> window size limit in current design, thus user level parameter
> sadb_sa_replay should honor this limit, otherwise misleading
> outputs("replay=244") by setkey -D will be:
>
> 192.168.25.2 192.168.22.2
> esp mode=transport spi=147561170(0x08cb9ad2) reqid=0(0x00000000)
> E: aes-cbc 9a8d7468 7655cf0b 719d27be b0ddaac2
> A: hmac-sha1 2d2115c2 ebf7c126 1c54f186 3b139b58 264a7331
> seq=0x00000000 replay=244 flags=0x00000000 state=mature
> created: Sep 17 14:00:00 2013 current: Sep 17 14:00:22 2013
> diff: 22(s) hard: 30(s) soft: 26(s)
> last: Sep 17 14:00:00 2013 hard: 0(s) soft: 0(s)
> current: 1408(bytes) hard: 0(bytes) soft: 0(bytes)
> allocated: 22 hard: 0 soft: 0
> sadb_seq=1 pid=4854 refcnt=0
> 192.168.22.2 192.168.25.2
> esp mode=transport spi=255302123(0x0f3799eb) reqid=0(0x00000000)
> E: aes-cbc 6485d990 f61a6bd5 e5660252 608ad282
> A: hmac-sha1 0cca811a eb4fa893 c47ae56c 98f6e413 87379a88
> seq=0x00000000 replay=244 flags=0x00000000 state=mature
> created: Sep 17 14:00:00 2013 current: Sep 17 14:00:22 2013
> diff: 22(s) hard: 30(s) soft: 26(s)
> last: Sep 17 14:00:00 2013 hard: 0(s) soft: 0(s)
> current: 1408(bytes) hard: 0(bytes) soft: 0(bytes)
> allocated: 22 hard: 0 soft: 0
> sadb_seq=0 pid=4854 refcnt=0
>
> And also, optimizing xfrm_replay_check window checking by setting the
> desirable x->props.replay_window with only doing the comparison once
> for all when xfrm_state is first born.
>
> Signed-off-by: Fan Du <fan.du@windriver.com>
Applied to ipsec, thanks!
^ permalink raw reply
* Re: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down
From: Wei Liu @ 2013-09-18 10:37 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, David Vrabel, Wei Liu, Ian Campbell
In-Reply-To: <1379436368-6882-1-git-send-email-paul.durrant@citrix.com>
On Tue, Sep 17, 2013 at 05:46:08PM +0100, Paul Durrant wrote:
> Without this patch, if a frontend cycles through states Closing
> and Closed (which Windows frontends need to do) then the netdev
> will be destroyed and requires re-invocation of hotplug scripts
> to restore state before the frontend can move to Connected. Thus
> when udev is not in use the backend gets stuck in InitWait.
>
> With this patch, the netdev is left alone whilst the backend is
> still online and is only de-registered and freed just prior to
> destroying the vif (which is also nicely symmetrical with the
> netdev allocation and registration being done during probe) so
> no re-invocation of hotplug scripts is required.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Thanks!
Wei.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox