* [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
* 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
* 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: [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: [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: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: [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: 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: [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: [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: [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: [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: 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: [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 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 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 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] 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: Ben Hutchings @ 2013-09-18 15:44 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
> Note that this patch must be applied with patch 4 (farsync_include_patch)
>
> Update the current farsync driver to support all of the PCI and PCI X
> cards manufactured by Farsite Communications.
>
> Add a tty interface so that the ports can also be used by the ppp daemon.
>
> Add support for big endian systems (the FarSite cards have a little endian
> processor on board).
[...]
Should all be separate patches. This is just doing way too much in one
go.
And there are regressions in here like:
> static struct pci_driver fst_driver = {
> - .name = FST_NAME,
> - .id_table = fst_pci_dev_id,
> - .probe = fst_add_one,
> - .remove = fst_remove_one,
> - .suspend = NULL,
> - .resume = NULL,
> +name: FST_NAME,
> +id_table : fst_pci_dev_id,
> +probe : fst_add_one,
> +remove : fst_remove_one,
> +suspend : NULL,
> +resume : NULL,
> +};
And the addition of a file in procfs (at the top level, even!) as if
it's still 2001 and sysfs doesn't exist:
> +static int farsync_proc_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, fst_proc_info, NULL);
> +}
> +
> +static const struct file_operations farsync_proc_fops = {
> + .open = farsync_proc_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> };
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] USBNET: fix handling padding packet
From: David Laight @ 2013-09-18 15:43 UTC (permalink / raw)
To: Bjørn Mork, Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman, Oliver Neukum, netdev,
linux-usb, Oliver Neukum
In-Reply-To: <878uyu6xjm.fsf@nemi.mork.no>
> 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;
> }
Actually it would probably be nicer if the minidriver set
a flag to indicate that it could support fragmented skb
(a lot can't because of the way they add trailers)
and also provided the length of the header it will add.
The usbnet code could then allocate the header space.
If scatter-gather dma is available (a host feature) then
the header can be allocated outside the skb data area
to avoid having to copy the entire skb data.
The check for ZLP avoidance could then be done once only
(not sure how the info would be passed to teh minidriver apart
from adding more additional parameters to teh tx_fixup function).
I did a quick scan of the minidrivers, some of them don't
seem to have the correct checks for fragmented skb (etc).
Most of them only add a header.
David
^ permalink raw reply
* Re: [PATCH] USBNET: fix handling padding packet
From: Bjørn Mork @ 2013-09-18 15:52 UTC (permalink / raw)
To: Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman, Oliver Neukum,
Network Development, linux-usb, Oliver Neukum
In-Reply-To: <CACVXFVNzTGbCV2wQ4RwxXK1q7+eD2KkOS_G3NvL4uAzdd21SUg@mail.gmail.com>
Ming Lei <ming.lei@canonical.com> writes:
>> 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().
Yes, I noticed that the driver doesn't set the flag. I just didn't put
too much into that. I don't think that necessarily means that the
padding is needed. It probably just means that the driver worked with
the default padding enabled, so the author left it there.
This flag should really have been inverted. ZLP should be the default
for all new usbnet drivers.
Why don't you test it on the device you tested the SG patch with? I am
pretty sure it works just fine using proper ZLP transfer termination.
>> "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?
Yes there is: Added complexity for everybody, based on a combination of
features which just does not make any sense.
No modern device should need the padding. No old device will be able to
use the SG feature as implemented. You only enable it on USB3, don't
you? If this feature is restricted to USB3 capable devices, then it most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.
Anyway, if you want to keep the padding for SG then maybe this will work
and allow you to drop the extra struct usbnet field and allocation:
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++], skb->data, 1);
}
I.e. cheat and use the skb->data buffer twice, if that is allowed? The
actual value of the padding byte should not matter, I believe?
Bjørn
^ permalink raw reply
* Re: [PATCH 006/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Ben Hutchings @ 2013-09-18 15:56 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: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA5@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:12 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 6 of 7
>
> Introduce the fsflex driver.
> This driver is functionally equivalent to the farsync driver, and so
> can be used with the Generic HDLC, and ppp daemon.
>
> 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/fsflex.c linux-3.10.1_new/drivers/net/wan/fsflex.c
> --- linux-3.10.1/drivers/net/wan/fsflex.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-3.10.1_new/drivers/net/wan/fsflex.c 2013-09-16 16:30:06.651104873 +0100
> @@ -0,0 +1,5693 @@
> +/*
> + * FarSync Flex driver for Linux (2.6.x kernel version)
Not exactly...
[...]
> +#define FST_MAX_CARDS 32
I don't think it's acceptable to set a static limit on number of devices
in a new driver.
[...]
> +int fst_txq_low = FST_LOW_WATER_MARK;
> +int fst_txq_high = FST_HIGH_WATER_MARK;
> +int fst_min_dma_len = FST_MIN_DMA_LEN;
> +int fst_dmathr = 0xdd00dd00;
> +int fst_iocinfo_version = FST_VERSION_CURRENT;
> +int fst_max_reads = 7;
> +int fst_excluded_cards;
> +int fst_excluded_list[FST_MAX_CARDS];
> +int fst_num_serials;
> +char *fst_serials[FST_MAX_CARDS];
All of which should be declared static, not implicitly extern. As
should all static variables and functions in this source file, as it's a
self-contained driver.
> +module_param(fst_txq_low, int, S_IRUGO);
> +module_param(fst_txq_high, int, S_IRUGO);
> +module_param(fst_min_dma_len, int, S_IRUGO);
> +module_param(fst_dmathr, int, S_IRUGO);
> +module_param(fst_iocinfo_version, int, S_IRUGO);
> +module_param(fst_max_reads, int, S_IRUGO);
> +module_param(fst_excluded_cards, int, S_IRUGO);
> +module_param_array(fst_excluded_list, int, NULL, S_IRUGO);
> +module_param_array(fst_serials, charp, &fst_num_serials, S_IRUGO);
Device configuration should be exposed through sysfs or ioctls, not
through module parameters.
[...]
> +#ifdef ARPHRD_RAWHDLC
> +#define ARPHRD_MYTYPE ARPHRD_RAWHDLC /* Raw frames */
> +#else
> +#define ARPHRD_MYTYPE ARPHRD_HDLC /* Cisco-HDLC (keepalives etc) */
> +#endif
#ifdef is not necessary in-tree.
[...]
> +#define usb_buffer_alloc usb_alloc_coherent
> +#define usb_buffer_free usb_free_coherent
Don't make up your own names for USB functions.
[...]
> +static int __init fst_init(void)
> +{
[...]
> + /* Create the /proc entry for /proc/fsflex
> + */
> + proc_create("fsflex", 0, NULL, &farsync_proc_fops);
[...]
Drivers should not be adding to the top level of /proc, and generally
should be adding any magic files under sysfs (per-device) or debugfs.
There is a /proc/driver directory if this information really doesn't fit
into either of those filesystems.
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 007/007] WAN Drivers: Update farsync driver and introduce fsflex driver
From: Ben Hutchings @ 2013-09-18 15:57 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: <E603DC592C92B54A89CEF6B0919A0B1CAAAA787DA6@SOLO.hq.farsitecommunications.com>
On Wed, 2013-09-18 at 11:12 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 7 of 7
>
> Update the help text and description for farsync configuration in the Kernel.
> Build farsync and fsflex when the farsync driver is selected.
fsflex seems to be an entirely independent module and should have its
own config option.
Ben.
> 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/Kconfig linux-3.10.1_new/drivers/net/wan/Kconfig
> --- linux-3.10.1/drivers/net/wan/Kconfig 2013-07-13 19:42:41.000000000 +0100
> +++ linux-3.10.1_new/drivers/net/wan/Kconfig 2013-07-31 14:11:05.792775800 +0100
> @@ -248,11 +248,11 @@ config C101
> If unsure, say N.
>
> config FARSYNC
> - tristate "FarSync T-Series support"
> + tristate "FarSync T-Series and Flex support"
> depends on HDLC && PCI
> ---help---
> - Support for the FarSync T-Series X.21 (and V.35/V.24) cards by
> - FarSite Communications Ltd.
> + Support for the FarSync T-Series and FarSync Flex X.21 (and
> + V.35/V.24) ports by FarSite Communications Ltd.
>
> Synchronous communication is supported on all ports at speeds up to
> 8Mb/s (128K on V.24) using synchronous PPP, Cisco HDLC, raw HDLC,
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/drivers/net/wan/Makefile linux-3.10.1_new/drivers/net/wan/Makefile
> --- linux-3.10.1/drivers/net/wan/Makefile 2013-07-13 19:42:41.000000000 +0100
> +++ linux-3.10.1_new/drivers/net/wan/Makefile 2013-07-26 09:14:15.345354002 +0100
> @@ -16,7 +16,7 @@ obj-$(CONFIG_HDLC_X25) += hdlc_x25.o
> obj-$(CONFIG_HOSTESS_SV11) += z85230.o hostess_sv11.o
> obj-$(CONFIG_SEALEVEL_4021) += z85230.o sealevel.o
> obj-$(CONFIG_COSA) += cosa.o
> -obj-$(CONFIG_FARSYNC) += farsync.o
> +obj-$(CONFIG_FARSYNC) += farsync.o fsflex.o
> obj-$(CONFIG_DSCC4) += dscc4.o
> obj-$(CONFIG_X25_ASY) += x25_asy.o
>
> --
> 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: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down
From: Ian Campbell @ 2013-09-18 15:58 UTC (permalink / raw)
To: Wei Liu; +Cc: Paul Durrant, xen-devel, netdev, David Vrabel
In-Reply-To: <20130918103704.GA511@zion.uk.xensource.com>
On Wed, 2013-09-18 at 11:37 +0100, Wei Liu wrote:
> 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>
yeah, looks good, thanks.
Paul, did you test this with non-Windows frontends too? and do things
like vif hot(un)plug still work?
Ian.
^ permalink raw reply
* RE: [PATCH net-next v4] Don't destroy the netdev until the vif is shut down
From: Paul Durrant @ 2013-09-18 16:04 UTC (permalink / raw)
To: Ian Campbell, Wei Liu
Cc: xen-devel@lists.xen.org, netdev@vger.kernel.org, David Vrabel
In-Reply-To: <1379519895.11304.268.camel@hastur.hellion.org.uk>
> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 18 September 2013 16:58
> To: Wei Liu
> Cc: Paul Durrant; xen-devel@lists.xen.org; netdev@vger.kernel.org; David
> Vrabel
> Subject: Re: [PATCH net-next v4] Don't destroy the netdev until the vif is
> shut down
>
> On Wed, 2013-09-18 at 11:37 +0100, Wei Liu wrote:
> > 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>
>
> yeah, looks good, thanks.
>
> Paul, did you test this with non-Windows frontends too? and do things
> like vif hot(un)plug still work?
>
Ian,
I tested with a debian (wheezy) PV guest but I didn't test unplug. I cycled a windows frontend several times (which is how I spotted the tx_irq thing), and shutdown and brought up both the debian and windows guests several times. I can test unplug too if you'd like.
Paul
^ 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