* RE: [PATCH v2 1/1] ip-link: in human readable output use dynamic precision length
From: David Laight @ 2014-11-05 9:30 UTC (permalink / raw)
To: 'Christian Hesse'; +Cc: Stephen Hemminger, netdev@vger.kernel.org
In-Reply-To: <20141104221038.440fd9d7@leda.localdomain>
From: Christian Hesse
...
>
> I do not like this at all... snprintf() would be nice for a catch-all, but we
> have to take care of negative values. So let's try something different.
> I will think about it and send a new patch.
>
> > > + precision = 0;
> > > +
> > > + snprintf(buf, sizeof(buf), "%.*f%c%s", precision,
> > > + (double) count / powi, *prefix, use_iec ? "i" : "");
> > >
> > > fprintf(fp, "%-*s ", width, buf);
> > > }
> >
> > The above will go wrong in all sorts of horrid ways....
> > For instance you are doing a truncating integer divide, but the FP
> > value will get rounded for display.
> >
> > It would be safer to use integers throughout.
>
> My implementation used integers, but Stephen changes this to floating point
> with his cleanups.
>
> IMHO the rounding is ok. This is for *human* readability. ;)
> Whoever wants correct values should not ask ip to print human readable values
> but rely on pure numbers.
Rounding the value is fine, the difference between 1000 and 1024
is not that great either.
The problem is that the two calculations get rounded differently.
If the FP value ends up being 9.999999 it will be printed as 10.0
but your field width calculation will only have allowed for
a single digit.
> > Oh, and a 2Mbit E1 link is actually 2048000 :-)
>
> Sorry? Did not get the point.
That was really in reference to a much earlier comment about comms
always using 1000.
David
^ permalink raw reply
* Re: [PATCH V1 3/4] can: add can_is_canfd_skb() API
From: Oliver Hartkopp @ 2014-11-05 9:39 UTC (permalink / raw)
To: Dong Aisheng, linux-can; +Cc: mkl, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <1415174326-6623-3-git-send-email-b29396@freescale.com>
On 05.11.2014 08:58, Dong Aisheng wrote:
> The CAN device drivers can use it to check if the frame to send is on
> CAN FD mode or normal CAN mode.
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
> include/linux/can/dev.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 6992afc..fe3be29 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -99,6 +99,11 @@ inval_skb:
> return 1;
> }
>
> +static inline int can_is_canfd_skb(struct sk_buff *skb)
> +{
> + return skb->protocol == htons(ETH_P_CANFD);
return skb->len == CANFD_MTU;
Please take the length as distinction as we do it in linux/net/can/ too.
You can add my
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
in the updated post then.
Regards,
Oliver
^ permalink raw reply
* [PATCH v2] stmmac: fix sparse warnings
From: Andy Shevchenko @ 2014-11-05 9:45 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev, David S . Miller, Vince Bridgers
Cc: Andy Shevchenko
This patch fixes the following sparse warnings.
drivers/net/ethernet/stmicro/stmmac/enh_desc.c:381:30: warning: symbol 'enh_desc_ops' was not declared. Should it be static?
drivers/net/ethernet/stmicro/stmmac/norm_desc.c:253:30: warning: symbol 'ndesc_ops' was not declared. Should it be static?
drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:141:33: warning: symbol 'stmmac_ptp' was not declared. Should it be static?
There is no functional change.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
---
Since v1:
- redone as Giuseppe suggested
drivers/net/ethernet/stmicro/stmmac/common.h | 5 +++++
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 4 +---
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 593e6c4..12a174e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -341,6 +341,9 @@ struct stmmac_desc_ops {
int (*get_rx_timestamp_status) (void *desc, u32 ats);
};
+extern const struct stmmac_desc_ops enh_desc_ops;
+extern const struct stmmac_desc_ops ndesc_ops;
+
struct stmmac_dma_ops {
/* DMA core initialization */
int (*init) (void __iomem *ioaddr, int pbl, int fb, int mb,
@@ -410,6 +413,8 @@ struct stmmac_hwtimestamp {
u64(*get_systime) (void __iomem *ioaddr);
};
+extern const struct stmmac_hwtimestamp stmmac_ptp;
+
struct mac_link {
int port;
int duplex;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index c3c4065..2d0036e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -122,9 +122,7 @@ int stmmac_mdio_unregister(struct net_device *ndev);
int stmmac_mdio_register(struct net_device *ndev);
int stmmac_mdio_reset(struct mii_bus *mii);
void stmmac_set_ethtool_ops(struct net_device *netdev);
-extern const struct stmmac_desc_ops enh_desc_ops;
-extern const struct stmmac_desc_ops ndesc_ops;
-extern const struct stmmac_hwtimestamp stmmac_ptp;
+
int stmmac_ptp_register(struct stmmac_priv *priv);
void stmmac_ptp_unregister(struct stmmac_priv *priv);
int stmmac_resume(struct net_device *ndev);
--
2.1.1
^ permalink raw reply related
* Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path
From: Ian Campbell @ 2014-11-05 9:51 UTC (permalink / raw)
To: David Miller
Cc: zoltan.kiss, david.vrabel, netdev, malcolm.crossley, wei.liu2,
xen-devel
In-Reply-To: <20141104.161704.1690311989900127361.davem@davemloft.net>
On Tue, 2014-11-04 at 16:17 -0500, David Miller wrote:
> From: Zoltan Kiss <zoltan.kiss@linaro.org>
> Date: Mon, 03 Nov 2014 18:23:03 +0000
>
> >
> >
> > On 03/11/14 17:46, David Vrabel wrote:
> >> On 03/11/14 17:39, Ian Campbell wrote:
> >>> On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
> >>>> From: Malcolm Crossley <malcolm.crossley@citrix.com>
> >>>>
> >>>> Unconditionally pulling 128 bytes into the linear buffer is not
> >>>> required. Netback has already grant copied up-to 128 bytes from the
> >>>> first slot of a packet into the linear buffer. The first slot normally
> >>>> contain all the IPv4/IPv6 and TCP/UDP headers.
> >>>
> >>> What about when it doesn't? It sounds as if we now won't pull up,
> >>> which
> >>> would be bad.
> >>
> >> The network stack will always pull any headers it needs to inspect
> >> (the
> >> frag may be a userspace page which has the same security issues as a
> >> frag with a foreign page).
> > I wouldn't bet my life on this, but indeed it should always happen.
>
> I would bet my life on it.
>
> Every protocol demux starts with pskb_may_pull() to pull frag data
> into the linear area, if necessary, before looking at headers.
Then I stand corrected, I was sure this wasn't the case (but my
information could well be a decade out of date...).
Is this also true for things which hit the iptables paths? I suppose
they must necessarily have already been through the protocol demux stage
before iptables would even be able to interpret them as e.g. an IP
packet.
Ian.
^ permalink raw reply
* Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path
From: Ian Campbell @ 2014-11-05 9:53 UTC (permalink / raw)
To: David Miller; +Cc: david.vrabel, netdev, xen-devel, wei.liu2, malcolm.crossley
In-Reply-To: <20141104.164113.2265592775058059992.davem@davemloft.net>
On Tue, 2014-11-04 at 16:41 -0500, David Miller wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> Date: Mon, 3 Nov 2014 17:23:51 +0000
>
> > From: Malcolm Crossley <malcolm.crossley@citrix.com>
> >
> > Unconditionally pulling 128 bytes into the linear buffer is not
> > required. Netback has already grant copied up-to 128 bytes from the
> > first slot of a packet into the linear buffer. The first slot normally
> > contain all the IPv4/IPv6 and TCP/UDP headers.
> >
> > The unconditional pull would often copy frag data unnecessarily. This
> > is a performance problem when running on a version of Xen where grant
> > unmap avoids TLB flushes for pages which are not accessed. TLB
> > flushes can now be avoided for > 99% of unmaps (it was 0% before).
> >
> > Grant unmap TLB flush avoidance will be available in a future version
> > of Xen (probably 4.6).
> >
> > Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> > Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>
> Now that this has been discussed a bit, it is possible to get an ack or two?
I'd like to see the commit message expanded to explain why this isn't
introducing a (security) bug by not pulling enough stuff into the header
(IOW the conclusion of the discussion).
Ian.
^ permalink raw reply
* Re: [PATCH net 2/2] geneve: Unregister pernet subsys on module unload.
From: Nicolas Dichtel @ 2014-11-05 10:08 UTC (permalink / raw)
To: Jesse Gross, David S. Miller; +Cc: netdev, Andy Zhou
In-Reply-To: <1415072318-64442-2-git-send-email-jesse@nicira.com>
Le 04/11/2014 04:38, Jesse Gross a écrit :
> The pernet ops aren't ever unregistered, which causes a memory
> leak and an OOPs if the module is ever reinserted.
>
> CC: Andy Zhou <azhou@nicira.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
Fixes: 0b5e8b8eeae4 ("net: Add Geneve tunneling protocol driver")
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
^ permalink raw reply
* Re: [PATCH 0/1] mv643xx_eth: Disable TSO by default
From: Karl Beldan @ 2014-11-05 10:09 UTC (permalink / raw)
To: Ian Campbell
Cc: Ezequiel Garcia, netdev, David Miller, Thomas Petazzoni,
Gregory Clement, Tawfik Bayouk, Lior Amsalem, Nadav Haklai,
764162
In-Reply-To: <1415176766.31613.7.camel@hellion.org.uk>
On Wed, Nov 05, 2014 at 08:39:26AM +0000, Ian Campbell wrote:
> On Tue, 2014-11-04 at 15:20 +0100, Karl Beldan wrote:
> > On Sat, Nov 01, 2014 at 12:30:19PM -0300, Ezequiel Garcia wrote:
> > > Several users ([1], [2]) have been reporting data corruption with TSO on
> > > Kirkwood platforms (i.e. using the mv643xx_eth driver).
> > >
> > > Until we manage to find what's causing this, this simple patch will make
> > > the TSO path disabled by default. This patch should be queued for stable,
> > > fixing the TSO feature introduced in v3.16.
> > >
> > > The corruption itself is very easy to reproduce: checking md5sum on a mounted
> > > NFS directory gives a different result each time. Same tests using the mvneta
> > > driver (Armada 370/38x/XP SoC) pass with no issues.
> > >
> > > Frankly, I'm a bit puzzled about this, and so any ideas or debugging hints
> > > are well received.
> > >
> >
> > Hi,
> >
> > Can you try this :
>
> It fixes things for me, thanks!
>
> Tested-by: Ian Campbell <ijc@hellion.org.uk>
>
Good thing, thanks for your feedbak Ian !
Karl
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Oliver Hartkopp @ 2014-11-05 10:12 UTC (permalink / raw)
To: Dong Aisheng, linux-can; +Cc: mkl, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <1415174326-6623-1-git-send-email-b29396@freescale.com>
On 05.11.2014 08:58, Dong Aisheng wrote:
> @@ -327,41 +357,65 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
> m_can_write(priv, M_CAN_ILE, 0x0);
> }
>
> -static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> - u32 rxfs)
> +static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
> {
> + struct net_device_stats *stats = &dev->stats;
> struct m_can_priv *priv = netdev_priv(dev);
> - u32 id, fgi;
> + struct canfd_frame *cf;
> + struct sk_buff *skb;
> + u32 id, fgi, dlc;
> + int i;
>
> /* calculate the fifo get index for where to read data */
> fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> + dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> + if (dlc & RX_BUF_EDL)
> + skb = alloc_canfd_skb(dev, &cf);
> + else
> + skb = alloc_can_skb(dev, (struct can_frame **)&cf);
Yes. That's the way it should look like ;-)
> + if (!skb) {
> + stats->rx_dropped++;
> + return;
> + }
> +
> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
> if (id & RX_BUF_XTD)
> cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> else
> cf->can_id = (id >> 18) & CAN_SFF_MASK;
>
> - if (id & RX_BUF_RTR) {
> + if (id & RX_BUF_ESI) {
> + cf->flags |= CANFD_ESI;
> + netdev_dbg(dev, "ESI Error\n");
> + }
> +
> + if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) {
> cf->can_id |= CAN_RTR_FLAG;
> } else {
> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> - cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
> - *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
> - M_CAN_FIFO_DATA(0));
> - *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
> - M_CAN_FIFO_DATA(1));
> + cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
if (dlc & RX_BUF_EDL)
cf->len = can_dlc2len((id >> 16) & 0x0F);
else
cf->len = get_can_dlc((id >> 16) & 0x0F);
(..)
> @@ -804,7 +870,8 @@ static void m_can_chip_config(struct net_device *dev)
> RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off);
>
> cccr = m_can_read(priv, M_CAN_CCCR);
> - cccr &= ~(CCCR_TEST | CCCR_MON);
> + cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
> + (CCCR_CME_MASK << CCCR_CME_SHIFT));
> test = m_can_read(priv, M_CAN_TEST);
> test &= ~TEST_LBCK;
>
> @@ -816,6 +883,9 @@ static void m_can_chip_config(struct net_device *dev)
> test |= TEST_LBCK;
> }
>
> + if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> + cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
> +
> m_can_write(priv, M_CAN_CCCR, cccr);
> m_can_write(priv, M_CAN_TEST, test);
>
(..)
>
> + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> + cccr = m_can_read(priv, M_CAN_CCCR);
> + cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
> + if (cf->flags & CANFD_BRS)
> + cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
> + else
> + cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
> + m_can_write(priv, M_CAN_CCCR, cccr);
> + }
Unfortunately No. Here it becomes complicated due to the fact that the
revision 3.0.x does not support per-frame switching for FD/BRS ...
When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* tells us, that
the controller is _capable_ to send either CAN or CAN FD frames.
It does not configure the controller into one of these specific settings!
Additionally: AFAIK when writing to the CCCR you have to make sure that
there's currently no ongoing transfer.
I would suggest the following approach to make the revision 3.0.x act like a
correct CAN FD capable controller:
- create a helper function to switch FD and BRS while taking care of ongoing
transmissions
- create a variable that knows the current tx_mode for FD and BRS
When we need to send a CAN frame which doesn't fit the settings in the tx_mode
we need to switch the CCCR and update the tx_mode variable.
This at least reduces the needed CCCR operations.
And it also addresses the intention of your patch
[PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode
Regards,
Oliver
^ permalink raw reply
* Re: [PATCH V2 2/4] can: m_can: workaround for transmit data less than 4 bytes
From: Marc Kleine-Budde @ 2014-11-05 10:17 UTC (permalink / raw)
To: Dong Aisheng, linux-can
Cc: wg, varkabhadram, netdev, socketcan, linux-arm-kernel
In-Reply-To: <1415174326-6623-2-git-send-email-b29396@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 2269 bytes --]
On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
Can you add the imx mask revision, too and the exact m_can version (is
available).
> the Message RAM was discovered. Sending CAN frames with dlc less
> than 4 bytes will lead to bit errors, when the first 8 bytes of
> the Message RAM have not been initialized (i.e. written to).
> To work around this issue, the first 8 bytes are initialized in open()
> function.
>
> Without the workaround, we can easily see the following errors:
> root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> [ 66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> root@imx6qdlsolo:~# cansend can0 123#112233
> [ 66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
> ChangeLog since v1:
> * initialize the first 8 bytes of Tx Buffer of Message RAM in open()
> to workaround the issue
> ---
> drivers/net/can/m_can/m_can.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 664fe30..f47c200 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -902,6 +902,15 @@ static void m_can_chip_config(struct net_device *dev)
> /* set bittiming params */
> m_can_set_bittiming(dev);
>
> + /* At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
> + * the Message RAM was discovered. Sending CAN frames with dlc less
> + * than 4 bytes will lead to bit errors, when the first 8 bytes of
> + * the Message RAM have not been initialized (i.e. written to).
> + * To work around this issue, the first 8 bytes are initialized here.
> + */
> + m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
> + m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
> +
> m_can_config_endisable(priv, false);
> }
>
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH v3 3/4] stmmac: pci: use managed resources
From: Andy Shevchenko @ 2014-11-05 10:27 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev, Kweh Hock Leong, David S . Miller,
Vince Bridgers
Cc: Andy Shevchenko
In-Reply-To: <1415183249-9231-1-git-send-email-andriy.shevchenko@linux.intel.com>
Migrate pci driver to managed resources to reduce boilerplate error handling
code.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 43 ++++++------------------
1 file changed, 10 insertions(+), 33 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index f19ac8e..5357a3f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -71,46 +71,37 @@ static void stmmac_default_data(void)
static int stmmac_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
- int ret = 0;
- void __iomem *addr = NULL;
- struct stmmac_priv *priv = NULL;
+ struct stmmac_priv *priv;
int i;
+ int ret;
/* Enable pci device */
- ret = pci_enable_device(pdev);
+ ret = pcim_enable_device(pdev);
if (ret) {
pr_err("%s : ERROR: failed to enable %s device\n", __func__,
pci_name(pdev));
return ret;
}
- if (pci_request_regions(pdev, STMMAC_RESOURCE_NAME)) {
- pr_err("%s: ERROR: failed to get PCI region\n", __func__);
- ret = -ENODEV;
- goto err_out_req_reg_failed;
- }
/* Get the base address of device */
for (i = 0; i <= PCI_STD_RESOURCE_END; i++) {
if (pci_resource_len(pdev, i) == 0)
continue;
- addr = pci_iomap(pdev, i, 0);
- if (addr == NULL) {
- pr_err("%s: ERROR: cannot map register memory aborting",
- __func__);
- ret = -EIO;
- goto err_out_map_failed;
- }
+ ret = pcim_iomap_regions(pdev, BIT(i), pci_name(pdev));
+ if (ret)
+ return ret;
break;
}
+
pci_set_master(pdev);
stmmac_default_data();
- priv = stmmac_dvr_probe(&(pdev->dev), &plat_dat, addr);
+ priv = stmmac_dvr_probe(&pdev->dev, &plat_dat,
+ pcim_iomap_table(pdev)[i]);
if (IS_ERR(priv)) {
pr_err("%s: main driver probe failed", __func__);
- ret = PTR_ERR(priv);
- goto err_out;
+ return PTR_ERR(priv);
}
priv->dev->irq = pdev->irq;
priv->wol_irq = pdev->irq;
@@ -120,15 +111,6 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
pr_debug("STMMAC platform driver registration completed");
return 0;
-
-err_out:
- pci_clear_master(pdev);
-err_out_map_failed:
- pci_release_regions(pdev);
-err_out_req_reg_failed:
- pci_disable_device(pdev);
-
- return ret;
}
/**
@@ -141,13 +123,8 @@ err_out_req_reg_failed:
static void stmmac_pci_remove(struct pci_dev *pdev)
{
struct net_device *ndev = pci_get_drvdata(pdev);
- struct stmmac_priv *priv = netdev_priv(ndev);
stmmac_dvr_remove(ndev);
-
- pci_iounmap(pdev, priv->ioaddr);
- pci_release_regions(pdev);
- pci_disable_device(pdev);
}
#ifdef CONFIG_PM_SLEEP
--
2.1.1
^ permalink raw reply related
* [PATCH v3 0/4] stmmac: pci: various cleanups and fixes
From: Andy Shevchenko @ 2014-11-05 10:27 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev, Kweh Hock Leong, David S . Miller,
Vince Bridgers
Cc: Andy Shevchenko
There are few cleanups and fixes regarding to stmmac PCI driver.
This has been tested on Intel Galileo board with recent net-next tree.
Since v2:
- drop patch 5/5 since it will be part of a big change across entire subsystem
Since v1:
- remove already applied patch
- append patch 1/5
- rework patch 3/5 to be functional compatible with original code
Andy Shevchenko (4):
stmmac: pci: use defined constant instead of magic number
stmmac: pci: convert to use dev_pm_ops
stmmac: pci: use managed resources
stmmac: pci: convert to use dev_* macros
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 80 ++++++++----------------
1 file changed, 26 insertions(+), 54 deletions(-)
--
2.1.1
^ permalink raw reply
* [PATCH v3 1/4] stmmac: pci: use defined constant instead of magic number
From: Andy Shevchenko @ 2014-11-05 10:27 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev, Kweh Hock Leong, David S . Miller,
Vince Bridgers
Cc: Andy Shevchenko
In-Reply-To: <1415183249-9231-1-git-send-email-andriy.shevchenko@linux.intel.com>
The last standard PCI resource is defined as PCI_STD_RESOURCE_END. Thus, we
could use it instead of plain integer.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index e17a970..e45794d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -90,7 +90,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
}
/* Get the base address of device */
- for (i = 0; i <= 5; i++) {
+ for (i = 0; i <= PCI_STD_RESOURCE_END; i++) {
if (pci_resource_len(pdev, i) == 0)
continue;
addr = pci_iomap(pdev, i, 0);
--
2.1.1
^ permalink raw reply related
* [PATCH v3 4/4] stmmac: pci: convert to use dev_* macros
From: Andy Shevchenko @ 2014-11-05 10:27 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev, Kweh Hock Leong, David S . Miller,
Vince Bridgers
Cc: Andy Shevchenko
In-Reply-To: <1415183249-9231-1-git-send-email-andriy.shevchenko@linux.intel.com>
Instead of pr_* macros let's use dev_* macros which provide device name.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 5357a3f..5084699 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -78,8 +78,8 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
/* Enable pci device */
ret = pcim_enable_device(pdev);
if (ret) {
- pr_err("%s : ERROR: failed to enable %s device\n", __func__,
- pci_name(pdev));
+ dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
+ __func__);
return ret;
}
@@ -100,7 +100,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
priv = stmmac_dvr_probe(&pdev->dev, &plat_dat,
pcim_iomap_table(pdev)[i]);
if (IS_ERR(priv)) {
- pr_err("%s: main driver probe failed", __func__);
+ dev_err(&pdev->dev, "%s: main driver probe failed\n", __func__);
return PTR_ERR(priv);
}
priv->dev->irq = pdev->irq;
@@ -108,7 +108,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
pci_set_drvdata(pdev, priv->dev);
- pr_debug("STMMAC platform driver registration completed");
+ dev_dbg(&pdev->dev, "STMMAC PCI driver registration completed\n");
return 0;
}
--
2.1.1
^ permalink raw reply related
* [PATCH v3 2/4] stmmac: pci: convert to use dev_pm_ops
From: Andy Shevchenko @ 2014-11-05 10:27 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev, Kweh Hock Leong, David S . Miller,
Vince Bridgers
Cc: Andy Shevchenko
In-Reply-To: <1415183249-9231-1-git-send-email-andriy.shevchenko@linux.intel.com>
Convert system PM callbacks to use dev_pm_ops. In addition remove the PCI calls
related to a power state since the bus code cares about this already.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 27 ++++++++++--------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index e45794d..f19ac8e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -150,30 +150,26 @@ static void stmmac_pci_remove(struct pci_dev *pdev)
pci_disable_device(pdev);
}
-#ifdef CONFIG_PM
-static int stmmac_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+#ifdef CONFIG_PM_SLEEP
+static int stmmac_pci_suspend(struct device *dev)
{
+ struct pci_dev *pdev = to_pci_dev(dev);
struct net_device *ndev = pci_get_drvdata(pdev);
- int ret;
- ret = stmmac_suspend(ndev);
- pci_save_state(pdev);
- pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
- return ret;
+ return stmmac_suspend(ndev);
}
-static int stmmac_pci_resume(struct pci_dev *pdev)
+static int stmmac_pci_resume(struct device *dev)
{
+ struct pci_dev *pdev = to_pci_dev(dev);
struct net_device *ndev = pci_get_drvdata(pdev);
- pci_set_power_state(pdev, PCI_D0);
- pci_restore_state(pdev);
-
return stmmac_resume(ndev);
}
#endif
+static SIMPLE_DEV_PM_OPS(stmmac_pm_ops, stmmac_pci_suspend, stmmac_pci_resume);
+
#define STMMAC_VENDOR_ID 0x700
#define STMMAC_DEVICE_ID 0x1108
@@ -190,10 +186,9 @@ struct pci_driver stmmac_pci_driver = {
.id_table = stmmac_id_table,
.probe = stmmac_pci_probe,
.remove = stmmac_pci_remove,
-#ifdef CONFIG_PM
- .suspend = stmmac_pci_suspend,
- .resume = stmmac_pci_resume,
-#endif
+ .driver = {
+ .pm = &stmmac_pm_ops,
+ },
};
MODULE_DESCRIPTION("STMMAC 10/100/1000 Ethernet PCI driver");
--
2.1.1
^ permalink raw reply related
* Re: [PATCH V2 2/4] can: m_can: workaround for transmit data less than 4 bytes
From: Dong Aisheng @ 2014-11-05 10:33 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: linux-can, wg, varkabhadram, netdev, socketcan, linux-arm-kernel
In-Reply-To: <5459F94C.1030506@pengutronix.de>
On Wed, Nov 05, 2014 at 11:17:48AM +0100, Marc Kleine-Budde wrote:
> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> > At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
>
> Can you add the imx mask revision, too and the exact m_can version (is
> available).
>
What do you mean imx mask revision?
By reading the Core Release Register (CREL), it seems the exact m_can
version is 3.0.1. (CREL = 30130506).
So what about changing to as follows?
"At least on the i.MX6SX with M_CAN IP version 3.0.1 (CREL = 30130506),
an issue with"
Regards
Dong Aisheng
> > the Message RAM was discovered. Sending CAN frames with dlc less
> > than 4 bytes will lead to bit errors, when the first 8 bytes of
> > the Message RAM have not been initialized (i.e. written to).
> > To work around this issue, the first 8 bytes are initialized in open()
> > function.
> >
> > Without the workaround, we can easily see the following errors:
> > root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> > [ 66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> > root@imx6qdlsolo:~# cansend can0 123#112233
> > [ 66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
> >
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> > ChangeLog since v1:
> > * initialize the first 8 bytes of Tx Buffer of Message RAM in open()
> > to workaround the issue
> > ---
> > drivers/net/can/m_can/m_can.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 664fe30..f47c200 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -902,6 +902,15 @@ static void m_can_chip_config(struct net_device *dev)
> > /* set bittiming params */
> > m_can_set_bittiming(dev);
> >
> > + /* At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
> > + * the Message RAM was discovered. Sending CAN frames with dlc less
> > + * than 4 bytes will lead to bit errors, when the first 8 bytes of
> > + * the Message RAM have not been initialized (i.e. written to).
> > + * To work around this issue, the first 8 bytes are initialized here.
> > + */
> > + m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
> > + m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
> > +
> > m_can_config_endisable(priv, false);
> > }
> >
> >
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
^ permalink raw reply
* Re: [PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode
From: Marc Kleine-Budde @ 2014-11-05 10:41 UTC (permalink / raw)
To: Dong Aisheng, linux-can
Cc: wg, varkabhadram, netdev, socketcan, linux-arm-kernel
In-Reply-To: <1415174326-6623-4-git-send-email-b29396@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]
On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> The current code sends all CAN frames on CAN FD format(with BRS or not)
> if CAN_CTRLMODE_FD is enabled.
> However, even CAN_CTRLMODE_FD is enabled, the can tool may still
> send normal frames.
> e.g.
> ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on
> cansend can0 123#112233
>
> Therefore sending normal CAN frame on FD format seems not reasonable
> and the CAN FD incapable device may not be able to receive it correctly.
>
> The patch switches the M_CAN operation mode to ISO11898-1 instead of
> staying on CAN FD operation mode by writing "11" to CMR bit if find
> we're sending a normal can skb.
With this patch applied and Olivre's version of 3/4, how does the
application send CAN-FD frames?
1. witch on CAN-FD via "ip fd on"
2. write a struct canfd_frame
Correct?
What happens if:
3. write a struct can_frame
A CAN frame is send?
Oliver are you okay with this behaviour?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Possible bug in net/core/neighbor.c
From: Ulf Samuelsson @ 2014-11-05 10:46 UTC (permalink / raw)
To: netdev
I find the following in "net/core/neighbor.c"
/* Compare new lladdr with cached one */
if (!dev->addr_len) {
/* First case: device needs no address. */
lladdr = neigh->ha;
} else if (lladdr) {
/* The second case: if something is already cached
and a new address is proposed:
- compare new & old
- if they are different, check override flag
*/
/* POSSIBLE BUG */
if ((old & NUD_VALID) &&
!memcmp(lladdr, neigh->ha, dev->addr_len))
lladdr = neigh->ha;
/* END POSSIBLE BUG */
} else {
/* No address is supplied; if we know something,
use it, otherwise discard the request.
*/
err = -EINVAL;
if (!(old & NUD_VALID))
goto out;
lladdr = neigh->ha;
}
It looks to me like the code is saying
if the proposed address is equal to the original address,
set the proposed address to the original address.
which is pretty meaningless.
Should it not be:
if ((old & NUD_VALID) &&
memcmp(lladdr, neigh->ha, dev->addr_len)) /* True if
addresses are not equal */
neigh->ha = lladdr; /* Update to new address */
If not, I would appreciate an explanation what the code is doing.
--
Best Regards,
Ulf Samuelsson
^ permalink raw reply
* Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path
From: David Vrabel @ 2014-11-05 10:46 UTC (permalink / raw)
To: Eric Dumazet, David Miller
Cc: zoltan.kiss, Ian.Campbell, netdev, malcolm.crossley, wei.liu2,
xen-devel
In-Reply-To: <1415137397.25370.7.camel@edumazet-glaptop2.roam.corp.google.com>
On 04/11/14 21:43, Eric Dumazet wrote:
> On Tue, 2014-11-04 at 16:17 -0500, David Miller wrote:
>
>
>>
>> Every protocol demux starts with pskb_may_pull() to pull frag data
>> into the linear area, if necessary, before looking at headers.
>
> eth_get_headlen() might be relevant as well, to perform a single copy of
> exactly all headers.
In netback's case we need an estimate of the header length before
reading any of the packet, since peeking at any frag would prevent any
TLB flush avoidance.
It might be useful to use eth_get_headlen() to adjust the estimate at
runtime, but for now the fixed amount of 128 bytes is simple and seems
good enough.
David
^ permalink raw reply
* [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path
From: David Vrabel @ 2014-11-05 10:50 UTC (permalink / raw)
To: netdev; +Cc: David Vrabel, xen-devel, Ian Campbell, Wei Liu, Malcolm Crossley
From: Malcolm Crossley <malcolm.crossley@citrix.com>
Unconditionally pulling 128 bytes into the linear area is not required
for:
- security: Every protocol demux starts with pskb_may_pull() to pull
frag data into the linear area, if necessary, before looking at
headers.
- performance: Netback has already grant copied up-to 128 bytes from
the first slot of a packet into the linear area. The first slot
normally contain all the IPv4/IPv6 and TCP/UDP headers.
The unconditional pull would often copy frag data unnecessarily. This
is a performance problem when running on a version of Xen where grant
unmap avoids TLB flushes for pages which are not accessed. TLB
flushes can now be avoided for > 99% of unmaps (it was 0% before).
Grant unmap TLB flush avoidance will be available in a future version
of Xen (probably 4.6).
Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
drivers/net/xen-netback/netback.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 730252c..14e18bb 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -82,6 +82,16 @@ MODULE_PARM_DESC(max_queues,
static unsigned int fatal_skb_slots = FATAL_SKB_SLOTS_DEFAULT;
module_param(fatal_skb_slots, uint, 0444);
+/* The amount to copy out of the first guest Tx slot into the skb's
+ * linear area. If the first slot has more data, it will be mapped
+ * and put into the first frag.
+ *
+ * This is sized to avoid pulling headers from the frags for most
+ * TCP/IP packets.
+ */
+#define XEN_NETBACK_TX_COPY_LEN 128
+
+
static void xenvif_idx_release(struct xenvif_queue *queue, u16 pending_idx,
u8 status);
@@ -125,13 +135,6 @@ static inline struct xenvif_queue *ubuf_to_queue(const struct ubuf_info *ubuf)
pending_tx_info[0]);
}
-/* This is a miniumum size for the linear area to avoid lots of
- * calls to __pskb_pull_tail() as we set up checksum offsets. The
- * value 128 was chosen as it covers all IPv4 and most likely
- * IPv6 headers.
- */
-#define PKT_PROT_LEN 128
-
static u16 frag_get_pending_idx(skb_frag_t *frag)
{
return (u16)frag->page_offset;
@@ -1446,9 +1449,9 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
index = pending_index(queue->pending_cons);
pending_idx = queue->pending_ring[index];
- data_len = (txreq.size > PKT_PROT_LEN &&
+ data_len = (txreq.size > XEN_NETBACK_TX_COPY_LEN &&
ret < XEN_NETBK_LEGACY_SLOTS_MAX) ?
- PKT_PROT_LEN : txreq.size;
+ XEN_NETBACK_TX_COPY_LEN : txreq.size;
skb = xenvif_alloc_skb(data_len);
if (unlikely(skb == NULL)) {
@@ -1653,11 +1656,6 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
}
}
- if (skb_is_nonlinear(skb) && skb_headlen(skb) < PKT_PROT_LEN) {
- int target = min_t(int, skb->len, PKT_PROT_LEN);
- __pskb_pull_tail(skb, target - skb_headlen(skb));
- }
-
skb->dev = queue->vif->dev;
skb->protocol = eth_type_trans(skb, skb->dev);
skb_reset_network_header(skb);
--
1.7.10.4
^ permalink raw reply related
* Re: [Xen-devel] [PATCHv1 net-next] xen-netback: remove unconditional pull_skb_tail in guest Tx path
From: Ian Campbell @ 2014-11-05 10:53 UTC (permalink / raw)
To: David Vrabel
Cc: Eric Dumazet, David Miller, zoltan.kiss, netdev, malcolm.crossley,
wei.liu2, xen-devel
In-Reply-To: <545A000D.8070808@citrix.com>
On Wed, 2014-11-05 at 10:46 +0000, David Vrabel wrote:
> On 04/11/14 21:43, Eric Dumazet wrote:
> > On Tue, 2014-11-04 at 16:17 -0500, David Miller wrote:
> >
> >
> >>
> >> Every protocol demux starts with pskb_may_pull() to pull frag data
> >> into the linear area, if necessary, before looking at headers.
> >
> > eth_get_headlen() might be relevant as well, to perform a single copy of
> > exactly all headers.
>
> In netback's case we need an estimate of the header length before
> reading any of the packet, since peeking at any frag would prevent any
> TLB flush avoidance.
>
> It might be useful to use eth_get_headlen() to adjust the estimate at
> runtime, but for now the fixed amount of 128 bytes is simple and seems
> good enough.
I think what Eric meant was that having done the 128 copy you could call
eth_get_headlen which in the common case should be a nop but would
ensure you always had the headers in the linear area for the uncommon
case.
It looks like the difference compared with skb_checksum_setup is that
eth_get_headlen deals with L4 too whereas skb_checksum_setup only goes
to L3 (and then only for some subset of protocols with checksums).
Ian.
^ permalink raw reply
* Re: [PATCHv2 net-next] xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path
From: Ian Campbell @ 2014-11-05 10:57 UTC (permalink / raw)
To: David Vrabel; +Cc: netdev, xen-devel, Wei Liu, Malcolm Crossley
In-Reply-To: <1415184622-19421-1-git-send-email-david.vrabel@citrix.com>
On Wed, 2014-11-05 at 10:50 +0000, David Vrabel wrote:
> From: Malcolm Crossley <malcolm.crossley@citrix.com>
>
> Unconditionally pulling 128 bytes into the linear area is not required
> for:
>
> - security: Every protocol demux starts with pskb_may_pull() to pull
> frag data into the linear area, if necessary, before looking at
> headers.
>
> - performance: Netback has already grant copied up-to 128 bytes from
> the first slot of a packet into the linear area. The first slot
> normally contain all the IPv4/IPv6 and TCP/UDP headers.
Thanks for adding these.
> The unconditional pull would often copy frag data unnecessarily. This
> is a performance problem when running on a version of Xen where grant
> unmap avoids TLB flushes for pages which are not accessed. TLB
> flushes can now be avoided for > 99% of unmaps (it was 0% before).
>
> Grant unmap TLB flush avoidance will be available in a future version
> of Xen (probably 4.6).
>
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply
* Re: [PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode
From: Oliver Hartkopp @ 2014-11-05 11:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Dong Aisheng, linux-can
Cc: wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <5459FEDE.9050405@pengutronix.de>
On 05.11.2014 11:41, Marc Kleine-Budde wrote:
> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
>> The current code sends all CAN frames on CAN FD format(with BRS or not)
>> if CAN_CTRLMODE_FD is enabled.
>> However, even CAN_CTRLMODE_FD is enabled, the can tool may still
>> send normal frames.
>> e.g.
>> ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on
>> cansend can0 123#112233
>>
>> Therefore sending normal CAN frame on FD format seems not reasonable
>> and the CAN FD incapable device may not be able to receive it correctly.
>>
>> The patch switches the M_CAN operation mode to ISO11898-1 instead of
>> staying on CAN FD operation mode by writing "11" to CMR bit if find
>> we're sending a normal can skb.
>
> With this patch applied and Olivre's version of 3/4, how does the
> application send CAN-FD frames?
This patch becomes obsolete when we do it like in my answer of [3/4].
> 1. witch on CAN-FD via "ip fd on"
With
ip link set can0 type can fd on
the netdevice switches to the MTU of CAN FD (72) instead of 16.
This means that this netdevice can handle CAN frames (MTU 16) and CAN FD
frames (MTU 72).
When you send a standard CAN frame, e.g.
cansend can0 123#112233
you would get a CAN 2.0 frame (dlc = 3) on the bus.
When you send a CAN FD frame, e.g.
cansend can0 123##0112233
you would get a CAN FD frame (dlc = 3) on the bus.
With
cansend can0 123##1112233
you would get a CAN FD frame (dlc = 3) with BRS on the bus.
Whether it is CAN or CAN FD is given by checking skb->len for CAN_MTU of
CANFD_MTU in the driver.
Regards,
Oliver
> 2. write a struct canfd_frame
>
> Correct?
>
> What happens if:
> 3. write a struct can_frame
>
> A CAN frame is send?
>
> Oliver are you okay with this behaviour?
>
> Marc
>
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Dong Aisheng @ 2014-11-05 11:26 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: linux-can, mkl, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <5459F808.3020903@hartkopp.net>
On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
> On 05.11.2014 08:58, Dong Aisheng wrote:
>
> >@@ -327,41 +357,65 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
> > m_can_write(priv, M_CAN_ILE, 0x0);
> > }
> >
> >-static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> >- u32 rxfs)
> >+static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
> > {
> >+ struct net_device_stats *stats = &dev->stats;
> > struct m_can_priv *priv = netdev_priv(dev);
> >- u32 id, fgi;
> >+ struct canfd_frame *cf;
> >+ struct sk_buff *skb;
> >+ u32 id, fgi, dlc;
> >+ int i;
> >
> > /* calculate the fifo get index for where to read data */
> > fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> >+ dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> >+ if (dlc & RX_BUF_EDL)
> >+ skb = alloc_canfd_skb(dev, &cf);
> >+ else
> >+ skb = alloc_can_skb(dev, (struct can_frame **)&cf);
>
> Yes. That's the way it should look like ;-)
>
> >+ if (!skb) {
> >+ stats->rx_dropped++;
> >+ return;
> >+ }
> >+
> > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
> > if (id & RX_BUF_XTD)
> > cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> > else
> > cf->can_id = (id >> 18) & CAN_SFF_MASK;
> >
> >- if (id & RX_BUF_RTR) {
> >+ if (id & RX_BUF_ESI) {
> >+ cf->flags |= CANFD_ESI;
> >+ netdev_dbg(dev, "ESI Error\n");
> >+ }
> >+
> >+ if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) {
> > cf->can_id |= CAN_RTR_FLAG;
> > } else {
> > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> >- cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
> >- *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
> >- M_CAN_FIFO_DATA(0));
> >- *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
> >- M_CAN_FIFO_DATA(1));
> >+ cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
>
> if (dlc & RX_BUF_EDL)
> cf->len = can_dlc2len((id >> 16) & 0x0F);
> else
> cf->len = get_can_dlc((id >> 16) & 0x0F);
>
> (..)
>
Correct.
Will change it.
> >@@ -804,7 +870,8 @@ static void m_can_chip_config(struct net_device *dev)
> > RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off);
> >
> > cccr = m_can_read(priv, M_CAN_CCCR);
> >- cccr &= ~(CCCR_TEST | CCCR_MON);
> >+ cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
> >+ (CCCR_CME_MASK << CCCR_CME_SHIFT));
> > test = m_can_read(priv, M_CAN_TEST);
> > test &= ~TEST_LBCK;
> >
> >@@ -816,6 +883,9 @@ static void m_can_chip_config(struct net_device *dev)
> > test |= TEST_LBCK;
> > }
> >
> >+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> >+ cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
> >+
> > m_can_write(priv, M_CAN_CCCR, cccr);
> > m_can_write(priv, M_CAN_TEST, test);
> >
>
> (..)
>
> >
> >+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> >+ cccr = m_can_read(priv, M_CAN_CCCR);
> >+ cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
> >+ if (cf->flags & CANFD_BRS)
> >+ cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
> >+ else
> >+ cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
> >+ m_can_write(priv, M_CAN_CCCR, cccr);
> >+ }
>
> Unfortunately No. Here it becomes complicated due to the fact that
> the revision 3.0.x does not support per-frame switching for FD/BRS
> ...
I'm not sure i got your point.
From m_can spec, it allows switch CAN mode by setting CMR bit.
Bits 11:10 CMR[1:0]: CAN Mode Request
A change of the CAN operation mode is requested by writing to this bit field. After change to the
requested operation mode the bit field is reset to “00” and the status flags FDBS and FDO are set
accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is
retained until it is overwritten by the next mode change request. In case CME = “01”/”10”/”11” a
change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation
according to ISO11898-1.
00 unchanged
01 Request CAN FD operation
10 Request CAN FD operation with bit rate switching
11 Request CAN operation according ISO11898-1
So what's the difference between this function and the per-frame switching
you mentioned?
>
> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
> tells us, that the controller is _capable_ to send either CAN or CAN
> FD frames.
>
> It does not configure the controller into one of these specific settings!
>
> Additionally: AFAIK when writing to the CCCR you have to make sure
> that there's currently no ongoing transfer.
>
I don't know about it before.
By searching m_can user manual v302 again, i still did not find this
limitation. Can you point me if you know where it is?
BTW, since we only use one Tx Buffer for transmission currently, so we
should not meet that case that CAN mode is switched during transfer.
So the issue you concern may not happen.
Additionally it looks to me like m_can does allow set CMR bit at runtime
since the mode change will take affect when controller becomes idle.
See below:
"A mode change requested by writing to CCCR.CMR will be executed next
time the CAN protocol controller FSM reaches idle phase between CAN frames.
Upon this event CCCR.CMR is reset to “00” and the status flags CCCR.FDBS
and CCCR.FDO are set accordingly. In case the requested
CAN operation mode is not enabled, the value written to CCCR.CMR is
retained until it is overwritten by the next mode change request.
Default is CAN operation according to ISO11898-1."
> I would suggest the following approach to make the revision 3.0.x
> act like a correct CAN FD capable controller:
>
> - create a helper function to switch FD and BRS while taking care of
> ongoing transmissions
>
> - create a variable that knows the current tx_mode for FD and BRS
>
> When we need to send a CAN frame which doesn't fit the settings in
> the tx_mode we need to switch the CCCR and update the tx_mode
> variable.
>
> This at least reduces the needed CCCR operations.
>
Yes, i can do like that.
But what i see by doing that is only reduces the needed CCCR operations?
Any other benefits i missed?
And the test for current code showed it seemed work well on the Mode
switch among std frame/fd frame/brs frame.
Regards
Dong Aisheng
> And it also addresses the intention of your patch
> [PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode
>
> Regards,
> Oliver
>
^ permalink raw reply
* Re: [PATCH V2 2/4] can: m_can: workaround for transmit data less than 4 bytes
From: Dong Aisheng @ 2014-11-05 11:32 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: linux-can, wg, varkabhadram, netdev, socketcan, linux-arm-kernel
In-Reply-To: <545A0AC5.80006@pengutronix.de>
On Wed, Nov 05, 2014 at 12:32:21PM +0100, Marc Kleine-Budde wrote:
> On 11/05/2014 11:33 AM, Dong Aisheng wrote:
> > On Wed, Nov 05, 2014 at 11:17:48AM +0100, Marc Kleine-Budde wrote:
> >> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> >>> At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
> >>
> >> Can you add the imx mask revision, too and the exact m_can version (is
> >> available).
> >>
> >
> > What do you mean imx mask revision?
>
> In my imx6sdl/imx6q data sheet it's called: Chip Silicon Version
> (USB_ANALOG_DIGPROG), I expect that the imx6sx has a the same register.
>
> > By reading the Core Release Register (CREL), it seems the exact m_can
> > version is 3.0.1. (CREL = 30130506).
> >
> > So what about changing to as follows?
> > "At least on the i.MX6SX with M_CAN IP version 3.0.1 (CREL = 30130506),
> > an issue with"
>
> Yes, please ass the imx silicon revision, too.
>
Okay, it's i.MX6SX TO1.2. Will add it.
Regards
Dong Aisheng
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
^ permalink raw reply
* Re: [PATCH V2 2/4] can: m_can: workaround for transmit data less than 4 bytes
From: Marc Kleine-Budde @ 2014-11-05 11:32 UTC (permalink / raw)
To: Dong Aisheng
Cc: linux-can, wg, varkabhadram, netdev, socketcan, linux-arm-kernel
In-Reply-To: <20141105103349.GA4007@shlinux1.ap.freescale.net>
[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]
On 11/05/2014 11:33 AM, Dong Aisheng wrote:
> On Wed, Nov 05, 2014 at 11:17:48AM +0100, Marc Kleine-Budde wrote:
>> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
>>> At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
>>
>> Can you add the imx mask revision, too and the exact m_can version (is
>> available).
>>
>
> What do you mean imx mask revision?
In my imx6sdl/imx6q data sheet it's called: Chip Silicon Version
(USB_ANALOG_DIGPROG), I expect that the imx6sx has a the same register.
> By reading the Core Release Register (CREL), it seems the exact m_can
> version is 3.0.1. (CREL = 30130506).
>
> So what about changing to as follows?
> "At least on the i.MX6SX with M_CAN IP version 3.0.1 (CREL = 30130506),
> an issue with"
Yes, please ass the imx silicon revision, too.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ 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