* Re: [PATCH 00/26] constify local structures
From: Jarkko Sakkinen @ 2016-09-11 17:21 UTC (permalink / raw)
To: Julia Lawall
Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Mustafa Ismail,
Tatyana Nikolova, kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-scsi-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-can-u79uwXL29TY76Z2rM5mHXA,
Shiraz Saleem, Sergei Shtylyov, netdev-u79uwXL29TY76Z2rM5mHXA,
Chien Tin Tung, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, joe-6d6DIl74uiNBDgjK7y7TUQ
In-Reply-To: <1473599168-30561-1-git-send-email-Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> Constify local structures.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
Just my two cents but:
1. You *can* use a static analysis too to find bugs or other issues.
2. However, you should manually do the commits and proper commit
messages to subsystems based on your findings. And I generally think
that if one contributes code one should also at least smoke test changes
somehow.
I don't know if I'm alone with my opinion. I just think that one should
also do the analysis part and not blindly create and submit patches.
Anyway, I'll apply the TPM change at some point. As I said they were
for better. Thanks.
/Jarkko
> // <smpl>
> // The first rule ignores some cases that posed problems
> @r disable optional_qualifier@
> identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
> identifier i != {s5k5baf_cis_rect,smtcfb_fix};
> position p;
> @@
> static struct s i@p = { ... };
>
> @lstruct@
> identifier r.s;
> @@
> struct s { ... };
>
> @used depends on lstruct@
> identifier r.i;
> @@
> i
>
> @bad1@
> expression e;
> identifier r.i;
> assignment operator a;
> @@
> (<+...i...+>) a e
>
> @bad2@
> identifier r.i;
> @@
> &(<+...i...+>)
>
> @bad3@
> identifier r.i;
> declarer d;
> @@
> d(...,<+...i...+>,...);
>
> @bad4@
> identifier r.i;
> type T;
> T[] e;
> identifier f;
> position p;
> @@
>
> f@p(...,
> (
> (<+...i...+>)
> &
> e
> )
> ,...)
>
> @bad4a@
> identifier r.i;
> type T;
> T *e;
> identifier f;
> position p;
> @@
>
> f@p(...,
> (
> (<+...i...+>)
> &
> e
> )
> ,...)
>
> @ok5@
> expression *e;
> identifier r.i;
> position p;
> @@
> e =@p i
>
> @bad5@
> expression *e;
> identifier r.i;
> position p != ok5.p;
> @@
> e =@p (<+...i...+>)
>
> @rr depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5@
> identifier s,r.i;
> position r.p;
> @@
>
> static
> +const
> struct s i@p = { ... };
>
> @depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5
> disable optional_qualifier@
> identifier rr.s,r.i;
> @@
>
> static
> +const
> struct s i;
> // </smpl>
>
> ---
>
> drivers/acpi/acpi_apd.c | 8 +++---
> drivers/char/tpm/tpm-interface.c | 10 ++++----
> drivers/char/tpm/tpm-sysfs.c | 2 -
> drivers/cpufreq/intel_pstate.c | 8 +++---
> drivers/infiniband/hw/i40iw/i40iw_uk.c | 6 ++---
> drivers/media/i2c/tvp514x.c | 2 -
> drivers/media/pci/ddbridge/ddbridge-core.c | 18 +++++++--------
> drivers/media/pci/ngene/ngene-cards.c | 14 ++++++------
> drivers/media/pci/smipcie/smipcie-main.c | 8 +++---
> drivers/misc/sgi-xp/xpc_uv.c | 2 -
> drivers/net/arcnet/com20020-pci.c | 10 ++++----
> drivers/net/can/c_can/c_can_pci.c | 4 +--
> drivers/net/can/sja1000/plx_pci.c | 20 ++++++++---------
> drivers/net/ethernet/mellanox/mlx4/main.c | 4 +--
> drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 2 -
> drivers/net/ethernet/renesas/sh_eth.c | 14 ++++++------
> drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 2 -
> drivers/net/wireless/ath/dfs_pattern_detector.c | 2 -
> drivers/net/wireless/intel/iwlegacy/3945.c | 4 +--
> drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c | 2 -
> drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c | 2 -
> drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c | 2 -
> drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c | 2 -
> drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c | 2 -
> drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c | 2 -
> drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c | 2 -
> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c | 2 -
> drivers/platform/chrome/chromeos_laptop.c | 22 +++++++++----------
> drivers/platform/x86/intel_scu_ipc.c | 6 ++---
> drivers/platform/x86/intel_telemetry_debugfs.c | 2 -
> drivers/scsi/esas2r/esas2r_flash.c | 2 -
> drivers/scsi/hptiop.c | 6 ++---
> drivers/spi/spi-dw-pci.c | 4 +--
> drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 2 -
> drivers/usb/misc/ezusb.c | 2 -
> drivers/video/fbdev/matrox/matroxfb_g450.c | 2 -
> lib/crc64_ecma.c | 2 -
> sound/pci/ctxfi/ctatc.c | 2 -
> sound/pci/hda/patch_ca0132.c | 10 ++++----
> sound/pci/riptide/riptide.c | 2 -
> 40 files changed, 110 insertions(+), 110 deletions(-)
------------------------------------------------------------------------------
^ permalink raw reply
* [PATCH 11/26] can: constify local structures
From: Julia Lawall @ 2016-09-11 13:05 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: joe, kernel-janitors, Marc Kleine-Budde, linux-can, netdev,
linux-kernel
In-Reply-To: <1473599168-30561-1-git-send-email-Julia.Lawall@lip6.fr>
For structure types defined in the same file or local header files, find
top-level static structure declarations that have the following
properties:
1. Never reassigned.
2. Address never taken
3. Not passed to a top-level macro call
4. No pointer or array-typed field passed to a function or stored in a
variable.
Declare structures having all of these properties as const.
Done using Coccinelle.
Based on a suggestion by Joe Perches <joe@perches.com>.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
The semantic patch seems too long for a commit log, but is in the cover
letter.
drivers/net/can/c_can/c_can_pci.c | 4 ++--
drivers/net/can/sja1000/plx_pci.c | 20 ++++++++++----------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 7be393c..4bc345d 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -251,14 +251,14 @@ static void c_can_pci_remove(struct pci_dev *pdev)
pci_disable_device(pdev);
}
-static struct c_can_pci_data c_can_sta2x11= {
+static const struct c_can_pci_data c_can_sta2x11 = {
.type = BOSCH_C_CAN,
.reg_align = C_CAN_REG_ALIGN_32,
.freq = 52000000, /* 52 Mhz */
.bar = 0,
};
-static struct c_can_pci_data c_can_pch = {
+static const struct c_can_pci_data c_can_pch = {
.type = BOSCH_C_CAN,
.reg_align = C_CAN_REG_32,
.freq = 50000000, /* 50 MHz */
diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
index 3eb7430..59bc378 100644
--- a/drivers/net/can/sja1000/plx_pci.c
+++ b/drivers/net/can/sja1000/plx_pci.c
@@ -170,7 +170,7 @@ struct plx_pci_card_info {
void (*reset_func)(struct pci_dev *pdev);
};
-static struct plx_pci_card_info plx_pci_card_info_adlink = {
+static const struct plx_pci_card_info plx_pci_card_info_adlink = {
"Adlink PCI-7841/cPCI-7841", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{1, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x80, 0x80} },
@@ -178,7 +178,7 @@ static struct plx_pci_card_info plx_pci_card_info_adlink = {
/* based on PLX9052 */
};
-static struct plx_pci_card_info plx_pci_card_info_adlink_se = {
+static const struct plx_pci_card_info plx_pci_card_info_adlink_se = {
"Adlink PCI-7841/cPCI-7841 SE", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x80, 0x80} },
@@ -186,7 +186,7 @@ static struct plx_pci_card_info plx_pci_card_info_adlink_se = {
/* based on PLX9052 */
};
-static struct plx_pci_card_info plx_pci_card_info_esd200 = {
+static const struct plx_pci_card_info plx_pci_card_info_esd200 = {
"esd CAN-PCI/CPCI/PCI104/200", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x100, 0x80} },
@@ -194,7 +194,7 @@ static struct plx_pci_card_info plx_pci_card_info_esd200 = {
/* based on PLX9030/9050 */
};
-static struct plx_pci_card_info plx_pci_card_info_esd266 = {
+static const struct plx_pci_card_info plx_pci_card_info_esd266 = {
"esd CAN-PCI/PMC/266", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x100, 0x80} },
@@ -202,7 +202,7 @@ static struct plx_pci_card_info plx_pci_card_info_esd266 = {
/* based on PLX9056 */
};
-static struct plx_pci_card_info plx_pci_card_info_esd2000 = {
+static const struct plx_pci_card_info plx_pci_card_info_esd2000 = {
"esd CAN-PCIe/2000", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x100, 0x80} },
@@ -210,7 +210,7 @@ static struct plx_pci_card_info plx_pci_card_info_esd2000 = {
/* based on PEX8311 */
};
-static struct plx_pci_card_info plx_pci_card_info_ixxat = {
+static const struct plx_pci_card_info plx_pci_card_info_ixxat = {
"IXXAT PC-I 04/PCI", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x200, 0x80} },
@@ -218,7 +218,7 @@ static struct plx_pci_card_info plx_pci_card_info_ixxat = {
/* based on PLX9050 */
};
-static struct plx_pci_card_info plx_pci_card_info_marathon_pci = {
+static const struct plx_pci_card_info plx_pci_card_info_marathon_pci = {
"Marathon CAN-bus-PCI", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x00, 0x00}, {4, 0x00, 0x00} },
@@ -234,7 +234,7 @@ static struct plx_pci_card_info plx_pci_card_info_marathon_pcie = {
/* based on PEX8311 */
};
-static struct plx_pci_card_info plx_pci_card_info_tews = {
+static const struct plx_pci_card_info plx_pci_card_info_tews = {
"TEWS TECHNOLOGIES TPMC810", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x000, 0x80}, {2, 0x100, 0x80} },
@@ -242,7 +242,7 @@ static struct plx_pci_card_info plx_pci_card_info_tews = {
/* based on PLX9030 */
};
-static struct plx_pci_card_info plx_pci_card_info_cti = {
+static const struct plx_pci_card_info plx_pci_card_info_cti = {
"Connect Tech Inc. CANpro/104-Plus Opto (CRG001)", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{0, 0x00, 0x00}, { {2, 0x000, 0x80}, {2, 0x100, 0x80} },
@@ -250,7 +250,7 @@ static struct plx_pci_card_info plx_pci_card_info_cti = {
/* based on PLX9030 */
};
-static struct plx_pci_card_info plx_pci_card_info_elcus = {
+static const struct plx_pci_card_info plx_pci_card_info_elcus = {
"Eclus CAN-200-PCI", 2,
PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
{1, 0x00, 0x00}, { {2, 0x00, 0x80}, {3, 0x00, 0x80} },
^ permalink raw reply related
* [PATCH 00/26] constify local structures
From: Julia Lawall @ 2016-09-11 13:05 UTC (permalink / raw)
To: linux-renesas-soc
Cc: alsa-devel, Mustafa Ismail, Tatyana Nikolova, kernel-janitors,
linux-fbdev, platform-driver-x86, devel, linux-scsi, linux-rdma,
Jason Gunthorpe, linux-acpi, tpmdd-devel, linux-media, linux-pm,
linux-can, Shiraz Saleem, Sergei Shtylyov, netdev, Chien Tin Tung,
linux-wireless, linux-kernel, linux-spi, linux-usb, joe
Constify local structures.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
// The first rule ignores some cases that posed problems
@r disable optional_qualifier@
identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
identifier i != {s5k5baf_cis_rect,smtcfb_fix};
position p;
@@
static struct s i@p = { ... };
@lstruct@
identifier r.s;
@@
struct s { ... };
@used depends on lstruct@
identifier r.i;
@@
i
@bad1@
expression e;
identifier r.i;
assignment operator a;
@@
(<+...i...+>) a e
@bad2@
identifier r.i;
@@
&(<+...i...+>)
@bad3@
identifier r.i;
declarer d;
@@
d(...,<+...i...+>,...);
@bad4@
identifier r.i;
type T;
T[] e;
identifier f;
position p;
@@
f@p(...,
(
(<+...i...+>)
&
e
)
,...)
@bad4a@
identifier r.i;
type T;
T *e;
identifier f;
position p;
@@
f@p(...,
(
(<+...i...+>)
&
e
)
,...)
@ok5@
expression *e;
identifier r.i;
position p;
@@
e =@p i
@bad5@
expression *e;
identifier r.i;
position p != ok5.p;
@@
e =@p (<+...i...+>)
@rr depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5@
identifier s,r.i;
position r.p;
@@
static
+const
struct s i@p = { ... };
@depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5
disable optional_qualifier@
identifier rr.s,r.i;
@@
static
+const
struct s i;
// </smpl>
---
drivers/acpi/acpi_apd.c | 8 +++---
drivers/char/tpm/tpm-interface.c | 10 ++++----
drivers/char/tpm/tpm-sysfs.c | 2 -
drivers/cpufreq/intel_pstate.c | 8 +++---
drivers/infiniband/hw/i40iw/i40iw_uk.c | 6 ++---
drivers/media/i2c/tvp514x.c | 2 -
drivers/media/pci/ddbridge/ddbridge-core.c | 18 +++++++--------
drivers/media/pci/ngene/ngene-cards.c | 14 ++++++------
drivers/media/pci/smipcie/smipcie-main.c | 8 +++---
drivers/misc/sgi-xp/xpc_uv.c | 2 -
drivers/net/arcnet/com20020-pci.c | 10 ++++----
drivers/net/can/c_can/c_can_pci.c | 4 +--
drivers/net/can/sja1000/plx_pci.c | 20 ++++++++---------
drivers/net/ethernet/mellanox/mlx4/main.c | 4 +--
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 2 -
drivers/net/ethernet/renesas/sh_eth.c | 14 ++++++------
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 2 -
drivers/net/wireless/ath/dfs_pattern_detector.c | 2 -
drivers/net/wireless/intel/iwlegacy/3945.c | 4 +--
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c | 2 -
drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c | 2 -
drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c | 2 -
drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c | 2 -
drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c | 2 -
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c | 2 -
drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c | 2 -
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c | 2 -
drivers/platform/chrome/chromeos_laptop.c | 22 +++++++++----------
drivers/platform/x86/intel_scu_ipc.c | 6 ++---
drivers/platform/x86/intel_telemetry_debugfs.c | 2 -
drivers/scsi/esas2r/esas2r_flash.c | 2 -
drivers/scsi/hptiop.c | 6 ++---
drivers/spi/spi-dw-pci.c | 4 +--
drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 2 -
drivers/usb/misc/ezusb.c | 2 -
drivers/video/fbdev/matrox/matroxfb_g450.c | 2 -
lib/crc64_ecma.c | 2 -
sound/pci/ctxfi/ctatc.c | 2 -
sound/pci/hda/patch_ca0132.c | 10 ++++----
sound/pci/riptide/riptide.c | 2 -
40 files changed, 110 insertions(+), 110 deletions(-)
^ permalink raw reply
* Re: [PATCH] CAN bus MIOe-3680 PCI (dual SJA1000 channel) emulation added.
From: Pavel Pisa @ 2016-09-08 17:10 UTC (permalink / raw)
To: Deniz Eren, linux-can; +Cc: qemu-devel, jasowang, rizzo, pbonzini
In-Reply-To: <1473145132-20895-1-git-send-email-deniz.eren@icloud.com>
Hello Deniz,
thanks much for contribution and testing.
I have applied your patches to our QEMU
repo
https://github.com/CTU-IIG/qemu
You find QEMU-2.6.1 based version of our merged
patches on "merged-2.6" branch.
I have updated and shortly tested all our topic branches
with actual QEMU-2.7.0 release. Simple test with
Linux within Linux and SocketCAN on both sides
works for PCI CAN-S (single SJA1000 channel) card
well with QEMU-2.7.0.
So I have undated all topics branches "can-pci",
"mf624" and "apohw" to this version then and created
"merged-2.7" branch. "merged" branch is now moved
to "merged-2.7" as well.
Your offer to help with mainlining is great as well.
Some discussion with somebody from core QEMU team
member is required.
I hope that actual CAN chip driver and PCI cards
follow QOM model right way. Problem is the simple
bus used for mesages delivery between CAN controllers.
These buses should be converted somehow to QOM acceptable
model. But for embedded development when you do not
freeze emulated system state is actual solution quite
usable as well.
It would be great to implement CAN FD (Flexible Datarate)
controller emulation support as well.
I am trying to find some diploma student for that for
two years already because he/she can have continuous
time for such work and testing which I cannot find.
Help from other instreested or project users would
be great as well.
Best wishes,
Pavel
^ permalink raw reply
* Re: [PATCH v3] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-08 13:54 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can@vger.kernel.org
In-Reply-To: <dced7a99-ade8-caf5-22e7-49be91577fbd@pengutronix.de>
On Thu, Sep 8, 2016 at 3:07 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 09/08/2016 10:06 AM, Nikita Edward Baruzdin wrote:
>> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
>> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>>
>> Thus, this patch introduces [read|write]_reg_rep() interface functions
>> that are used to read/write CAN ID and payload data.
>>
>> * For plx_pci driver this interface is implemented as
>> plx_pci_[read|write]_reg_rep() that use ioread32()/iowrite32() for
>> register accesses as much as possible to improve driver performance.
>>
>> * For other drivers the default implementation,
>> sja1000_[read|write]_reg_rep(), is used for now. These functions
>> still access registers in a series of ioread8()/iowrite8() calls.
>>
>> Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
>
> Please compile with C=2 and fix these warnings. You need to cast to
> __XXYY if you want to use it in XXYY_to_cpu.
I actually needed to compile with C=2 CF=-D__CHECK_ENDIAN__. Fixed
now, see the new version.
>> CHECK /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:326:32: warning: incorrect type in assignment (different base types)
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:326:32: expected unsigned int [unsigned] [usertype] <noident>
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:326:32: got restricted __be32 [usertype] <noident>
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:331:32: warning: incorrect type in assignment (different base types)
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:331:32: expected unsigned short [unsigned] [short] [usertype] <noident>
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:331:32: got restricted __be16 [usertype] <noident>
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:374:22: warning: cast to restricted __be32
>> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:380:22: warning: cast to restricted __be16
>
> 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
* [PATCH v4] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-08 13:51 UTC (permalink / raw)
To: linux-can
Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
instead of consecutive ioread8()/iowrite8() calls seems reasonable.
Thus, this patch introduces [read|write]_reg_rep() interface functions
that are used to read/write CAN ID and payload data.
* For plx_pci driver this interface is implemented as
plx_pci_[read|write]_reg_rep() that use ioread32()/iowrite32() for
register accesses as much as possible to improve driver performance.
* For other drivers the default implementation,
sja1000_[read|write]_reg_rep(), is used for now. These functions
still access registers in a series of ioread8()/iowrite8() calls.
Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
---
drivers/net/can/sja1000/plx_pci.c | 38 +++++++++++++++++++++++++++
drivers/net/can/sja1000/sja1000.c | 55 ++++++++++++++++++++++++---------------
drivers/net/can/sja1000/sja1000.h | 4 +++
3 files changed, 76 insertions(+), 21 deletions(-)
diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
index 3eb7430..ff2858f 100644
--- a/drivers/net/can/sja1000/plx_pci.c
+++ b/drivers/net/can/sja1000/plx_pci.c
@@ -371,6 +371,42 @@ static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 val)
iowrite8(val, priv->reg_base + port);
}
+static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
+ u8 *buffer, size_t count)
+{
+ while (count >= 4) {
+ *(__le32 *)buffer = cpu_to_le32(ioread32(priv->reg_base + reg));
+ buffer += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ *(__le16 *)buffer = cpu_to_le16(ioread16(priv->reg_base + reg));
+ buffer += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ *buffer = ioread8(priv->reg_base + reg);
+}
+
+static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
+ const u8 *buffer, size_t count)
+{
+ while (count >= 4) {
+ iowrite32(le32_to_cpu(*(__le32 *)buffer), priv->reg_base + reg);
+ buffer += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ iowrite16(le16_to_cpu(*(__le16 *)buffer), priv->reg_base + reg);
+ buffer += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ iowrite8(*buffer, priv->reg_base + reg);
+}
+
/*
* Check if a CAN controller is present at the specified location
* by trying to switch 'em from the Basic mode into the PeliCAN mode.
@@ -626,6 +662,8 @@ static int plx_pci_add_card(struct pci_dev *pdev,
priv->reg_base = addr + cm->offset;
priv->read_reg = plx_pci_read_reg;
priv->write_reg = plx_pci_write_reg;
+ priv->read_reg_rep = plx_pci_read_reg_rep;
+ priv->write_reg_rep = plx_pci_write_reg_rep;
/* Check if channel is present */
if (plx_pci_check_sja1000(priv)) {
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 9f10779..ff046f3 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -82,6 +82,24 @@ static const struct can_bittiming_const sja1000_bittiming_const = {
.brp_inc = 1,
};
+static void sja1000_read_reg_rep(const struct sja1000_priv *priv, int reg,
+ u8 *buffer, size_t count)
+{
+ int i;
+
+ for (i = 0; i < count; i++)
+ buffer[i] = priv->read_reg(priv, reg + i);
+}
+
+static void sja1000_write_reg_rep(const struct sja1000_priv *priv, int reg,
+ const u8 *buffer, size_t count)
+{
+ int i;
+
+ for (i = 0; i < count; i++)
+ priv->write_reg(priv, reg + i, buffer[i]);
+}
+
static void sja1000_write_cmdreg(struct sja1000_priv *priv, u8 val)
{
unsigned long flags;
@@ -288,7 +306,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
canid_t id;
uint8_t dreg;
u8 cmd_reg_val = 0x00;
- int i;
+ u8 id_buf[4];
if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
@@ -305,19 +323,16 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
fi |= SJA1000_FI_FF;
dreg = SJA1000_EFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
- priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
- priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
+ *(__be32 *)id_buf = cpu_to_be32(id << 3);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
} else {
dreg = SJA1000_SFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
+ *(__be16 *)id_buf = cpu_to_be16(id << 5);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 2);
}
- for (i = 0; i < dlc; i++)
- priv->write_reg(priv, dreg++, cf->data[i]);
+ priv->write_reg_rep(priv, dreg, cf->data, dlc);
can_put_echo_skb(skb, dev, 0);
@@ -343,7 +358,7 @@ static void sja1000_rx(struct net_device *dev)
uint8_t fi;
uint8_t dreg;
canid_t id;
- int i;
+ u8 id_buf[4];
/* create zero'ed CAN frame buffer */
skb = alloc_can_skb(dev, &cf);
@@ -355,25 +370,21 @@ static void sja1000_rx(struct net_device *dev)
if (fi & SJA1000_FI_FF) {
/* extended frame format (EFF) */
dreg = SJA1000_EFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 21)
- | (priv->read_reg(priv, SJA1000_ID2) << 13)
- | (priv->read_reg(priv, SJA1000_ID3) << 5)
- | (priv->read_reg(priv, SJA1000_ID4) >> 3);
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 4);
+ id = be32_to_cpu(*(__be32 *)id_buf) >> 3;
id |= CAN_EFF_FLAG;
} else {
/* standard frame format (SFF) */
dreg = SJA1000_SFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 3)
- | (priv->read_reg(priv, SJA1000_ID2) >> 5);
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 2);
+ id = be16_to_cpu(*(__be16 *)id_buf) >> 5;
}
cf->can_dlc = get_can_dlc(fi & 0x0F);
- if (fi & SJA1000_FI_RTR) {
+ if (fi & SJA1000_FI_RTR)
id |= CAN_RTR_FLAG;
- } else {
- for (i = 0; i < cf->can_dlc; i++)
- cf->data[i] = priv->read_reg(priv, dreg++);
- }
+ else
+ priv->read_reg_rep(priv, dreg, cf->data, cf->can_dlc);
cf->can_id = id;
@@ -639,6 +650,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
CAN_CTRLMODE_ONE_SHOT |
CAN_CTRLMODE_BERR_REPORTING |
CAN_CTRLMODE_PRESUME_ACK;
+ priv->read_reg_rep = sja1000_read_reg_rep;
+ priv->write_reg_rep = sja1000_write_reg_rep;
spin_lock_init(&priv->cmdreg_lock);
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398..68d5e5f 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -157,6 +157,10 @@ struct sja1000_priv {
/* the lower-layer is responsible for appropriate locking */
u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
+ void (*read_reg_rep) (const struct sja1000_priv *priv, int reg,
+ u8 *buffer, size_t count);
+ void (*write_reg_rep) (const struct sja1000_priv *priv, int reg,
+ const u8 *buffer, size_t count);
void (*pre_irq) (const struct sja1000_priv *priv);
void (*post_irq) (const struct sja1000_priv *priv);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v3] can: sja1000: Optimise register accesses
From: Marc Kleine-Budde @ 2016-09-08 12:07 UTC (permalink / raw)
To: Nikita Edward Baruzdin, linux-can
In-Reply-To: <20160908080613.3997-1-nebaruzdin@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 2341 bytes --]
On 09/08/2016 10:06 AM, Nikita Edward Baruzdin wrote:
> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>
> Thus, this patch introduces [read|write]_reg_rep() interface functions
> that are used to read/write CAN ID and payload data.
>
> * For plx_pci driver this interface is implemented as
> plx_pci_[read|write]_reg_rep() that use ioread32()/iowrite32() for
> register accesses as much as possible to improve driver performance.
>
> * For other drivers the default implementation,
> sja1000_[read|write]_reg_rep(), is used for now. These functions
> still access registers in a series of ioread8()/iowrite8() calls.
>
> Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
Please compile with C=2 and fix these warnings. You need to cast to
__XXYY if you want to use it in XXYY_to_cpu.
> CHECK /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:326:32: warning: incorrect type in assignment (different base types)
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:326:32: expected unsigned int [unsigned] [usertype] <noident>
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:326:32: got restricted __be32 [usertype] <noident>
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:331:32: warning: incorrect type in assignment (different base types)
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:331:32: expected unsigned short [unsigned] [short] [usertype] <noident>
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:331:32: got restricted __be16 [usertype] <noident>
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:374:22: warning: cast to restricted __be32
> /srv/work/frogger/socketcan/linux/drivers/net/can/sja1000/sja1000.c:380:22: warning: cast to restricted __be16
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: 455 bytes --]
^ permalink raw reply
* Re: [PATCH v3] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-08 11:10 UTC (permalink / raw)
To: Alexander Gerasiov; +Cc: linux-can@vger.kernel.org
In-Reply-To: <20160908113607.7b7e7ca8@brick.gerasiov.net>
On Thu, Sep 8, 2016 at 11:36 AM, Alexander Gerasiov <gq@redlab-i.ru> wrote:
> Hello Nikita,
>
> On Thu, 8 Sep 2016 11:06:13 +0300
> Nikita Edward Baruzdin <nebaruzdin@gmail.com> wrote:
>
>> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
>> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>>
>> Thus, this patch introduces [read|write]_reg_rep() interface functions
>> that are used to read/write CAN ID and payload data.
>>
>> * For plx_pci driver this interface is implemented as
>> plx_pci_[read|write]_reg_rep() that use ioread32()/iowrite32() for
>> register accesses as much as possible to improve driver
>> performance.
>>
>> * For other drivers the default implementation,
>> sja1000_[read|write]_reg_rep(), is used for now. These functions
>> still access registers in a series of ioread8()/iowrite8() calls.
>
> This looks OK for me. (I hope you didn't forget to run tests with this
> variant. And with default implementation of rep functions too.)
Sure, I've tested both plx_pci_[read|write]_reg_rep() and
sja1000_[read|write]_reg_rep() implementations, meaning that candump shows the
same frame info for both sender and receiver CAN interfaces, as I specify in a
cansend command. I've also checked that results from the table in my first
email are still true.
> It would be good if someone run tests with other plx_pci based adapters
> (they should also work with 32 bit access via PCI, but who knows).
> Tests with other sja1000 cards and on big-endian boxes are welcome too.
>
>
>
> --
> Best regards,
> Alexander Gerasiov
>
> Contacts:
> e-mail: gq@cs.msu.su Homepage: http://gerasiov.net Skype: gerasiov
> PGP fingerprint: 04B5 9D90 DF7C C2AB CD49 BAEA CA87 E9E8 2AAC 33F1
^ permalink raw reply
* Re: [PATCH v3] can: sja1000: Optimise register accesses
From: Alexander Gerasiov @ 2016-09-08 8:36 UTC (permalink / raw)
To: Nikita Edward Baruzdin; +Cc: linux-can
In-Reply-To: <20160908080613.3997-1-nebaruzdin@gmail.com>
Hello Nikita,
On Thu, 8 Sep 2016 11:06:13 +0300
Nikita Edward Baruzdin <nebaruzdin@gmail.com> wrote:
> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>
> Thus, this patch introduces [read|write]_reg_rep() interface functions
> that are used to read/write CAN ID and payload data.
>
> * For plx_pci driver this interface is implemented as
> plx_pci_[read|write]_reg_rep() that use ioread32()/iowrite32() for
> register accesses as much as possible to improve driver
> performance.
>
> * For other drivers the default implementation,
> sja1000_[read|write]_reg_rep(), is used for now. These functions
> still access registers in a series of ioread8()/iowrite8() calls.
This looks OK for me. (I hope you didn't forget to run tests with this
variant. And with default implementation of rep functions too.)
It would be good if someone run tests with other plx_pci based adapters
(they should also work with 32 bit access via PCI, but who knows).
Tests with other sja1000 cards and on big-endian boxes are welcome too.
--
Best regards,
Alexander Gerasiov
Contacts:
e-mail: gq@cs.msu.su Homepage: http://gerasiov.net Skype: gerasiov
PGP fingerprint: 04B5 9D90 DF7C C2AB CD49 BAEA CA87 E9E8 2AAC 33F1
^ permalink raw reply
* Re: [PATCH v2] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-08 8:11 UTC (permalink / raw)
To: Alexander Gerasiov; +Cc: linux-can@vger.kernel.org
In-Reply-To: <20160907183316.0c2fdc4f@brick.gerasiov.net>
On Wed, Sep 7, 2016 at 6:33 PM, Alexander Gerasiov <gq@redlab-i.ru> wrote:
> Hello Nikita,
>
> On Wed, 7 Sep 2016 13:38:52 +0300
> Nikita Edward Baruzdin <nebaruzdin@gmail.com> wrote:
>
>> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
>> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>>
>> Thus, this patch introduces plx_pci_read_reg_rep() and
>> plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
>> register accesses as much as possible. The functions are then used to
>> read/write CAN ID and payload data to improve driver performance.
>>
>> Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
>
>> @@ -305,19 +306,33 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>> fi |= SJA1000_FI_FF;
>> dreg = SJA1000_EFF_BUF;
>> priv->write_reg(priv, SJA1000_FI, fi);
>> - priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
>> - priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
>> - priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
>> - priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
>> + if (priv->write_reg_rep) {
>> + *(u32 *)id_buf = cpu_to_be32(id << 3);
>> + priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
>> + } else {
>> + priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
>> + priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
>> + priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
>> + priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
>> + }
>
> Nikita, as I said before, I believe that this should be implemented in
> slightly different way.
>
> sja1000.c should have
> static void sja1000_read_reg_rep(u8 *buf, size_t size)
> {
> unsigned pos = 0;
> while(pos < size){
> priv->read_reg(buf[pos]);
> pos++;
> }
> }
>
> and priv->read_reg_rep = sja1000_read_reg_rep;
>
> But in plx_pci.c you implement optimized version
> static void plx_pci_read_reg_rep()
> {
> }
>
> and assign it to priv->read_reg_rep
>
> The same thing should be done with write functions.
>
> Then you call _reg_rep functions everywhere in the code (where you read/write
> more then one register).
I think this indeed should be a clearer approach, thank you. See the
following patch version.
>
> --
> Best regards,
> Alexander Gerasiov
>
> Contacts:
> e-mail: gq@cs.msu.su Homepage: http://gerasiov.net Skype: gerasiov
> PGP fingerprint: 04B5 9D90 DF7C C2AB CD49 BAEA CA87 E9E8 2AAC 33F1
^ permalink raw reply
* [PATCH v3] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-08 8:06 UTC (permalink / raw)
To: linux-can
Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
instead of consecutive ioread8()/iowrite8() calls seems reasonable.
Thus, this patch introduces [read|write]_reg_rep() interface functions
that are used to read/write CAN ID and payload data.
* For plx_pci driver this interface is implemented as
plx_pci_[read|write]_reg_rep() that use ioread32()/iowrite32() for
register accesses as much as possible to improve driver performance.
* For other drivers the default implementation,
sja1000_[read|write]_reg_rep(), is used for now. These functions
still access registers in a series of ioread8()/iowrite8() calls.
Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
---
drivers/net/can/sja1000/plx_pci.c | 38 +++++++++++++++++++++++++++
drivers/net/can/sja1000/sja1000.c | 55 ++++++++++++++++++++++++---------------
drivers/net/can/sja1000/sja1000.h | 4 +++
3 files changed, 76 insertions(+), 21 deletions(-)
diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
index 3eb7430..a401c0e 100644
--- a/drivers/net/can/sja1000/plx_pci.c
+++ b/drivers/net/can/sja1000/plx_pci.c
@@ -371,6 +371,42 @@ static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 val)
iowrite8(val, priv->reg_base + port);
}
+static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
+ u8 *buffer, size_t count)
+{
+ while (count >= 4) {
+ *(u32 *)buffer = cpu_to_le32(ioread32(priv->reg_base + reg));
+ buffer += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ *(u16 *)buffer = cpu_to_le16(ioread16(priv->reg_base + reg));
+ buffer += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ *buffer = ioread8(priv->reg_base + reg);
+}
+
+static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
+ const u8 *buffer, size_t count)
+{
+ while (count >= 4) {
+ iowrite32(le32_to_cpu(*(u32 *)buffer), priv->reg_base + reg);
+ buffer += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ iowrite16(le16_to_cpu(*(u16 *)buffer), priv->reg_base + reg);
+ buffer += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ iowrite8(*buffer, priv->reg_base + reg);
+}
+
/*
* Check if a CAN controller is present at the specified location
* by trying to switch 'em from the Basic mode into the PeliCAN mode.
@@ -626,6 +662,8 @@ static int plx_pci_add_card(struct pci_dev *pdev,
priv->reg_base = addr + cm->offset;
priv->read_reg = plx_pci_read_reg;
priv->write_reg = plx_pci_write_reg;
+ priv->read_reg_rep = plx_pci_read_reg_rep;
+ priv->write_reg_rep = plx_pci_write_reg_rep;
/* Check if channel is present */
if (plx_pci_check_sja1000(priv)) {
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 9f10779..c1acfa4 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -82,6 +82,24 @@ static const struct can_bittiming_const sja1000_bittiming_const = {
.brp_inc = 1,
};
+static void sja1000_read_reg_rep(const struct sja1000_priv *priv, int reg,
+ u8 *buffer, size_t count)
+{
+ int i;
+
+ for (i = 0; i < count; i++)
+ buffer[i] = priv->read_reg(priv, reg + i);
+}
+
+static void sja1000_write_reg_rep(const struct sja1000_priv *priv, int reg,
+ const u8 *buffer, size_t count)
+{
+ int i;
+
+ for (i = 0; i < count; i++)
+ priv->write_reg(priv, reg + i, buffer[i]);
+}
+
static void sja1000_write_cmdreg(struct sja1000_priv *priv, u8 val)
{
unsigned long flags;
@@ -288,7 +306,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
canid_t id;
uint8_t dreg;
u8 cmd_reg_val = 0x00;
- int i;
+ u8 id_buf[4];
if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
@@ -305,19 +323,16 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
fi |= SJA1000_FI_FF;
dreg = SJA1000_EFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
- priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
- priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
+ *(u32 *)id_buf = cpu_to_be32(id << 3);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
} else {
dreg = SJA1000_SFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
+ *(u16 *)id_buf = cpu_to_be16(id << 5);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 2);
}
- for (i = 0; i < dlc; i++)
- priv->write_reg(priv, dreg++, cf->data[i]);
+ priv->write_reg_rep(priv, dreg, cf->data, dlc);
can_put_echo_skb(skb, dev, 0);
@@ -343,7 +358,7 @@ static void sja1000_rx(struct net_device *dev)
uint8_t fi;
uint8_t dreg;
canid_t id;
- int i;
+ u8 id_buf[4];
/* create zero'ed CAN frame buffer */
skb = alloc_can_skb(dev, &cf);
@@ -355,25 +370,21 @@ static void sja1000_rx(struct net_device *dev)
if (fi & SJA1000_FI_FF) {
/* extended frame format (EFF) */
dreg = SJA1000_EFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 21)
- | (priv->read_reg(priv, SJA1000_ID2) << 13)
- | (priv->read_reg(priv, SJA1000_ID3) << 5)
- | (priv->read_reg(priv, SJA1000_ID4) >> 3);
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 4);
+ id = be32_to_cpu(*(u32 *)id_buf) >> 3;
id |= CAN_EFF_FLAG;
} else {
/* standard frame format (SFF) */
dreg = SJA1000_SFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 3)
- | (priv->read_reg(priv, SJA1000_ID2) >> 5);
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 2);
+ id = be16_to_cpu(*(u16 *)id_buf) >> 5;
}
cf->can_dlc = get_can_dlc(fi & 0x0F);
- if (fi & SJA1000_FI_RTR) {
+ if (fi & SJA1000_FI_RTR)
id |= CAN_RTR_FLAG;
- } else {
- for (i = 0; i < cf->can_dlc; i++)
- cf->data[i] = priv->read_reg(priv, dreg++);
- }
+ else
+ priv->read_reg_rep(priv, dreg, cf->data, cf->can_dlc);
cf->can_id = id;
@@ -639,6 +650,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
CAN_CTRLMODE_ONE_SHOT |
CAN_CTRLMODE_BERR_REPORTING |
CAN_CTRLMODE_PRESUME_ACK;
+ priv->read_reg_rep = sja1000_read_reg_rep;
+ priv->write_reg_rep = sja1000_write_reg_rep;
spin_lock_init(&priv->cmdreg_lock);
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398..68d5e5f 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -157,6 +157,10 @@ struct sja1000_priv {
/* the lower-layer is responsible for appropriate locking */
u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
+ void (*read_reg_rep) (const struct sja1000_priv *priv, int reg,
+ u8 *buffer, size_t count);
+ void (*write_reg_rep) (const struct sja1000_priv *priv, int reg,
+ const u8 *buffer, size_t count);
void (*pre_irq) (const struct sja1000_priv *priv);
void (*post_irq) (const struct sja1000_priv *priv);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2] can: sja1000: Optimise register accesses
From: Alexander Gerasiov @ 2016-09-07 15:33 UTC (permalink / raw)
To: Nikita Edward Baruzdin, linux-can
In-Reply-To: <20160907103852.28795-1-nebaruzdin@gmail.com>
Hello Nikita,
On Wed, 7 Sep 2016 13:38:52 +0300
Nikita Edward Baruzdin <nebaruzdin@gmail.com> wrote:
> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>
> Thus, this patch introduces plx_pci_read_reg_rep() and
> plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
> register accesses as much as possible. The functions are then used to
> read/write CAN ID and payload data to improve driver performance.
>
> Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
> @@ -305,19 +306,33 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
> fi |= SJA1000_FI_FF;
> dreg = SJA1000_EFF_BUF;
> priv->write_reg(priv, SJA1000_FI, fi);
> - priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
> - priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
> - priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
> - priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
> + if (priv->write_reg_rep) {
> + *(u32 *)id_buf = cpu_to_be32(id << 3);
> + priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
> + } else {
> + priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
> + priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
> + priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
> + priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
> + }
Nikita, as I said before, I believe that this should be implemented in
slightly different way.
sja1000.c should have
static void sja1000_read_reg_rep(u8 *buf, size_t size)
{
unsigned pos = 0;
while(pos < size){
priv->read_reg(buf[pos]);
pos++;
}
}
and priv->read_reg_rep = sja1000_read_reg_rep;
But in plx_pci.c you implement optimized version
static void plx_pci_read_reg_rep()
{
}
and assign it to priv->read_reg_rep
The same thing should be done with write functions.
Then you call _reg_rep functions everywhere in the code (where you read/write
more then one register).
--
Best regards,
Alexander Gerasiov
Contacts:
e-mail: gq@cs.msu.su Homepage: http://gerasiov.net Skype: gerasiov
PGP fingerprint: 04B5 9D90 DF7C C2AB CD49 BAEA CA87 E9E8 2AAC 33F1
^ permalink raw reply
* [PATCH 1/1] can: fix deadlock reported after bus-off
From: Sergei Miroshnichenko @ 2016-09-07 13:51 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Cc: Alexander Dyachenko, Stefano Babic, Sergei Miroshnichenko
A timer was used to restart after the bus-off state, leading to a
relatively large can_restart() executed in an interrupt context,
which in turn sets up pinctrl. When this happens during system boot,
there is a high probability of grabbing the pinctrl_list_mutex,
which is locked already by the probe() of other device, making the
kernel suspect a deadlock condition [1].
To resolve this issue, the restart_timer is replaced by a delayed
work.
[1] https://github.com/victronenergy/venus/issues/24
Signed-off-by: Sergei Miroshnichenko <sergeimir@emcraft.com>
---
drivers/net/can/dev.c | 27 +++++++++++++++++----------
include/linux/can/dev.h | 3 ++-
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e21f7cc..8d6208c 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -21,6 +21,7 @@
#include <linux/slab.h>
#include <linux/netdevice.h>
#include <linux/if_arp.h>
+#include <linux/workqueue.h>
#include <linux/can.h>
#include <linux/can/dev.h>
#include <linux/can/skb.h>
@@ -501,9 +502,8 @@ EXPORT_SYMBOL_GPL(can_free_echo_skb);
/*
* CAN device restart for bus-off recovery
*/
-static void can_restart(unsigned long data)
+static void can_restart(struct net_device *dev)
{
- struct net_device *dev = (struct net_device *)data;
struct can_priv *priv = netdev_priv(dev);
struct net_device_stats *stats = &dev->stats;
struct sk_buff *skb;
@@ -543,6 +543,14 @@ restart:
netdev_err(dev, "Error %d during restart", err);
}
+static void can_restart_work(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct can_priv *priv = container_of(dwork, struct can_priv, restart_work);
+
+ can_restart(priv->dev);
+}
+
int can_restart_now(struct net_device *dev)
{
struct can_priv *priv = netdev_priv(dev);
@@ -556,8 +564,8 @@ int can_restart_now(struct net_device *dev)
if (priv->state != CAN_STATE_BUS_OFF)
return -EBUSY;
- /* Runs as soon as possible in the timer context */
- mod_timer(&priv->restart_timer, jiffies);
+ cancel_delayed_work_sync(&priv->restart_work);
+ can_restart(dev);
return 0;
}
@@ -578,8 +586,8 @@ void can_bus_off(struct net_device *dev)
netif_carrier_off(dev);
if (priv->restart_ms)
- mod_timer(&priv->restart_timer,
- jiffies + (priv->restart_ms * HZ) / 1000);
+ schedule_delayed_work(&priv->restart_work,
+ msecs_to_jiffies(priv->restart_ms));
}
EXPORT_SYMBOL_GPL(can_bus_off);
@@ -688,6 +696,7 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
return NULL;
priv = netdev_priv(dev);
+ priv->dev = dev;
if (echo_skb_max) {
priv->echo_skb_max = echo_skb_max;
@@ -697,7 +706,7 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
priv->state = CAN_STATE_STOPPED;
- init_timer(&priv->restart_timer);
+ INIT_DELAYED_WORK(&priv->restart_work, can_restart_work);
return dev;
}
@@ -778,8 +787,6 @@ int open_candev(struct net_device *dev)
if (!netif_carrier_ok(dev))
netif_carrier_on(dev);
- setup_timer(&priv->restart_timer, can_restart, (unsigned long)dev);
-
return 0;
}
EXPORT_SYMBOL_GPL(open_candev);
@@ -794,7 +801,7 @@ void close_candev(struct net_device *dev)
{
struct can_priv *priv = netdev_priv(dev);
- del_timer_sync(&priv->restart_timer);
+ cancel_delayed_work_sync(&priv->restart_work);
can_flush_echo_skb(dev);
}
EXPORT_SYMBOL_GPL(close_candev);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5261751..5f527094 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -32,6 +32,7 @@ enum can_mode {
* CAN common private data
*/
struct can_priv {
+ struct net_device *dev;
struct can_device_stats can_stats;
struct can_bittiming bittiming, data_bittiming;
@@ -47,7 +48,7 @@ struct can_priv {
u32 ctrlmode_static; /* static enabled options for driver/hardware */
int restart_ms;
- struct timer_list restart_timer;
+ struct delayed_work restart_work;
int (*do_set_bittiming)(struct net_device *dev);
int (*do_set_data_bittiming)(struct net_device *dev);
--
2.1.0
^ permalink raw reply related
* [PATCH v2] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-07 10:38 UTC (permalink / raw)
To: linux-can
Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
instead of consecutive ioread8()/iowrite8() calls seems reasonable.
Thus, this patch introduces plx_pci_read_reg_rep() and
plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
register accesses as much as possible. The functions are then used to
read/write CAN ID and payload data to improve driver performance.
Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
---
drivers/net/can/sja1000/plx_pci.c | 42 ++++++++++++++++++++++++++
drivers/net/can/sja1000/sja1000.c | 62 +++++++++++++++++++++++++++++----------
drivers/net/can/sja1000/sja1000.h | 4 +++
3 files changed, 92 insertions(+), 16 deletions(-)
diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
index 3eb7430..c319bbe 100644
--- a/drivers/net/can/sja1000/plx_pci.c
+++ b/drivers/net/can/sja1000/plx_pci.c
@@ -371,6 +371,46 @@ static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 val)
iowrite8(val, priv->reg_base + port);
}
+static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
+ void *buffer, unsigned long count)
+{
+ u8 *p = buffer;
+
+ while (count >= 4) {
+ *(u32 *)p = cpu_to_le32(ioread32(priv->reg_base + reg));
+ p += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ *(u16 *)p = cpu_to_le16(ioread16(priv->reg_base + reg));
+ p += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ *p = ioread8(priv->reg_base + reg);
+}
+
+static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
+ const void *buffer, unsigned long count)
+{
+ const u8 *p = buffer;
+
+ while (count >= 4) {
+ iowrite32(le32_to_cpu(*(u32 *)p), priv->reg_base + reg);
+ p += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ iowrite16(le16_to_cpu(*(u16 *)p), priv->reg_base + reg);
+ p += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ iowrite8(*p, priv->reg_base + reg);
+}
+
/*
* Check if a CAN controller is present at the specified location
* by trying to switch 'em from the Basic mode into the PeliCAN mode.
@@ -626,6 +666,8 @@ static int plx_pci_add_card(struct pci_dev *pdev,
priv->reg_base = addr + cm->offset;
priv->read_reg = plx_pci_read_reg;
priv->write_reg = plx_pci_write_reg;
+ priv->read_reg_rep = plx_pci_read_reg_rep;
+ priv->write_reg_rep = plx_pci_write_reg_rep;
/* Check if channel is present */
if (plx_pci_check_sja1000(priv)) {
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 9f10779..ca1766d 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -288,6 +288,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
canid_t id;
uint8_t dreg;
u8 cmd_reg_val = 0x00;
+ u8 id_buf[4];
int i;
if (can_dropped_invalid_skb(dev, skb))
@@ -305,19 +306,33 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
fi |= SJA1000_FI_FF;
dreg = SJA1000_EFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
- priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
- priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
+ if (priv->write_reg_rep) {
+ *(u32 *)id_buf = cpu_to_be32(id << 3);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
+ } else {
+ priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
+ priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
+ priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
+ priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
+ }
} else {
dreg = SJA1000_SFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
+ if (priv->write_reg_rep) {
+ *(u16 *)id_buf = cpu_to_be16(id << 5);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 2);
+ } else {
+ priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
+ priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
+ }
}
- for (i = 0; i < dlc; i++)
- priv->write_reg(priv, dreg++, cf->data[i]);
+ if (priv->write_reg_rep) {
+ priv->write_reg_rep(priv, dreg, cf->data, dlc);
+ } else {
+ for (i = 0; i < dlc; i++)
+ priv->write_reg(priv, dreg++, cf->data[i]);
+ }
can_put_echo_skb(skb, dev, 0);
@@ -343,6 +358,7 @@ static void sja1000_rx(struct net_device *dev)
uint8_t fi;
uint8_t dreg;
canid_t id;
+ u8 id_buf[4];
int i;
/* create zero'ed CAN frame buffer */
@@ -355,24 +371,38 @@ static void sja1000_rx(struct net_device *dev)
if (fi & SJA1000_FI_FF) {
/* extended frame format (EFF) */
dreg = SJA1000_EFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 21)
- | (priv->read_reg(priv, SJA1000_ID2) << 13)
- | (priv->read_reg(priv, SJA1000_ID3) << 5)
- | (priv->read_reg(priv, SJA1000_ID4) >> 3);
+ if (priv->read_reg_rep) {
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 4);
+ id = be32_to_cpu(*(u32 *)id_buf) >> 3;
+ } else {
+ id = (priv->read_reg(priv, SJA1000_ID1) << 21)
+ | (priv->read_reg(priv, SJA1000_ID2) << 13)
+ | (priv->read_reg(priv, SJA1000_ID3) << 5)
+ | (priv->read_reg(priv, SJA1000_ID4) >> 3);
+ }
id |= CAN_EFF_FLAG;
} else {
/* standard frame format (SFF) */
dreg = SJA1000_SFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 3)
- | (priv->read_reg(priv, SJA1000_ID2) >> 5);
+ if (priv->read_reg_rep) {
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 2);
+ id = be16_to_cpu(*(u16 *)id_buf) >> 5;
+ } else {
+ id = (priv->read_reg(priv, SJA1000_ID1) << 3)
+ | (priv->read_reg(priv, SJA1000_ID2) >> 5);
+ }
}
cf->can_dlc = get_can_dlc(fi & 0x0F);
if (fi & SJA1000_FI_RTR) {
id |= CAN_RTR_FLAG;
} else {
- for (i = 0; i < cf->can_dlc; i++)
- cf->data[i] = priv->read_reg(priv, dreg++);
+ if (priv->read_reg_rep) {
+ priv->read_reg_rep(priv, dreg, cf->data, cf->can_dlc);
+ } else {
+ for (i = 0; i < cf->can_dlc; i++)
+ cf->data[i] = priv->read_reg(priv, dreg++);
+ }
}
cf->can_id = id;
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398..887045a 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -157,6 +157,10 @@ struct sja1000_priv {
/* the lower-layer is responsible for appropriate locking */
u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
+ void (*read_reg_rep) (const struct sja1000_priv *priv, int reg,
+ void *buffer, unsigned long count);
+ void (*write_reg_rep) (const struct sja1000_priv *priv, int reg,
+ const void *buffer, unsigned long count);
void (*pre_irq) (const struct sja1000_priv *priv);
void (*post_irq) (const struct sja1000_priv *priv);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2 00/12] can: rx-offload: add implmentation and switch flexcan driver to use it
From: Holger Schurig @ 2016-09-07 6:33 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
In-Reply-To: <1467657137-18891-1-git-send-email-mkl@pengutronix.de>
Marc Kleine-Budde <mkl@pengutronix.de> writes:
Late answer :-)
This driver was tested with Linux 4.7.2 an on i.MX6Q device. Not by me,
I don't even have their exact test software. I just get the reports ...
Using some external tool (maybe Kvaser?) they generated 80% busload at
500 kbps, the driver survived for 3 days without any data loss. So,
compared with the current in-kernel driver, this is a definitive
improvement!
Then they did same CAN ping-ping test with reboots and wrote: "Out of
about 16,000 reboots we had one failure. This failed both CAN interfaces
and the logs show that the PTX-C received all CAN messages and returned
them, but the test machine for some reason did not receive any CAN
messages."
I don't have any idea why Linux thought it was sending but the receiver
didn't receive anything. And I'm no CAN expert, not at all. Wouldn't I
get an error condition on the sending side? How would a program get
this error, is it a return value to sendto()
Holger
^ permalink raw reply
* Re: [PATCH] can: sja1000: Optimise register accesses
From: Oliver Hartkopp @ 2016-09-06 19:18 UTC (permalink / raw)
To: Nikita Edward Baruzdin; +Cc: linux-can@vger.kernel.org
In-Reply-To: <CAEJ7JO1BRcMux59srO6VMW8jBXn8ycMy_U-ir2ke65pzXrS+UQ@mail.gmail.com>
On 09/06/2016 06:22 PM, Nikita Edward Baruzdin wrote:
> On Mon, Sep 5, 2016 at 10:56 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> You always take care of the CPU endian when reading writing CAN identifiers.
>> Why is it ok to use read_reg_rep() for the CAN data content without any
>> beXX_to_cpu() conversion?
>
> beXX_to_cpu() is there just because CAN ID is stored in device registers in the
> big-endian manner. The old code actually does the same byte reordering manually
> with shifts.
>
> However, I've now realised that current plx_pci_read/write_reg_rep()
> implementations won't work with big-endian CPUs as ioreadXX()/iowriteXX()
> assume the driver data needs to be cpu-endian while it really needs to be
> little-endian since it is treated as a byte array.
>
> Will it be ok, if I add cpu_to_leXX()/leXX_to_cpu() and change functions like
> this? I've checked this is still correct with Intel CPU.
I assume this will be ok :-)
Please send an updated version, so that Marc can take a look too.
Best regards,
Oliver
^ permalink raw reply
* Fast Loans
From: Financial Service @ 2016-09-06 15:12 UTC (permalink / raw)
To: Recipients
Apply for a loan at 2% reply to this Email for more Info
^ permalink raw reply
* Re: [PATCH] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-06 16:22 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can@vger.kernel.org
In-Reply-To: <9a11af52-faf8-b784-8387-9ee48a6ed744@hartkopp.net>
On Mon, Sep 5, 2016 at 10:56 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hello Nikita,
>
> On 09/05/2016 07:58 PM, Nikita Edward Baruzdin wrote:
>>
>> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
>> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
>
>
> I definitely like this patch.
>
>> Thus, this patch introduces plx_pci_read_reg_rep() and
>> plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
>> register accesses as much as possible. The functions are then used to
>> read/write CAN ID and payload data to improve driver performance.
>
>
> But there is something more to do as you currently brake all sja1000 drivers
> except plx_pci ;-)
>
> You need to take care of drivers that do not support
> priv->[read|write]_reg_rep().
>
> So that other (pci) drivers can follow at will after testing.
Thanks! I'm sorry for being so sloppy :/
I will resend the patch.
> Additionally:
>
> You always take care of the CPU endian when reading writing CAN identifiers.
> Why is it ok to use read_reg_rep() for the CAN data content without any
> beXX_to_cpu() conversion?
beXX_to_cpu() is there just because CAN ID is stored in device registers in the
big-endian manner. The old code actually does the same byte reordering manually
with shifts.
However, I've now realised that current plx_pci_read/write_reg_rep()
implementations won't work with big-endian CPUs as ioreadXX()/iowriteXX()
assume the driver data needs to be cpu-endian while it really needs to be
little-endian since it is treated as a byte array.
Will it be ok, if I add cpu_to_leXX()/leXX_to_cpu() and change functions like
this? I've checked this is still correct with Intel CPU.
>static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
> void *buffer, unsigned long count)
>{
> u8 *p = buffer;
>
> while (count >= 4) {
> *(u32 *)p = cpu_to_le32(ioread32(priv->reg_base + reg));
> p += 4;
> reg += 4;
> count -= 4;
> }
> if (count & 2) {
> *(u16 *)p = cpu_to_le16(ioread16(priv->reg_base + reg));
> p += 2;
> reg += 2;
> }
> if (count & 1)
> *p = ioread8(priv->reg_base + reg);
>}
>
>static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
> const void *buffer, unsigned long count)
>{
> const u8 *p = buffer;
>
> while (count >= 4) {
> iowrite32(le32_to_cpu(*(u32 *)p), priv->reg_base + reg);
> p += 4;
> reg += 4;
> count -= 4;
> }
> if (count & 2) {
> iowrite16(le16_to_cpu(*(u16 *)p), priv->reg_base + reg);
> p += 2;
> reg += 2;
> }
> if (count & 1)
> iowrite8(*p, priv->reg_base + reg);
>}
^ permalink raw reply
* [PATCH] can-j1939: fix memdup_user.cocci warnings
From: kbuild test robot @ 2016-09-06 11:58 UTC (permalink / raw)
To: Kurt Van Dijck; +Cc: kbuild-all, linux-can, Marc Kleine-Budde
In-Reply-To: <201609061930.NHHial3C%fengguang.wu@intel.com>
net/can/j1939/socket.c:510:13-20: WARNING opportunity for memdup_user
Use memdup_user rather than duplicating its implementation
This is a little bit restricted to reduce false positives
Generated by: scripts/coccinelle/api/memdup_user.cocci
CC: Kurt Van Dijck <kurt.van.dijck@eia.be>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
0 files changed
rule starting on line 17: position variables or mixed modifs interfere with comm_assoc isobool (
(
(
(unknown *to == NULL)
>>> IS_ERR(rule starting on line 17:to)
|
!unknown *to
>>> IS_ERR(rule starting on line 17:to)
)
|
(unknown *NULL == unknown *to)
>>> IS_ERR(rule starting on line 17:to)
) || ...)
^ permalink raw reply
* [mkl-can-next:j1939 1/1] net/can/j1939/socket.c:707:2: error: too few arguments to function 'sock_tx_timestamp'
From: kbuild test robot @ 2016-09-06 11:58 UTC (permalink / raw)
To: Kurt Van Dijck; +Cc: kbuild-all, linux-can, Marc Kleine-Budde
[-- Attachment #1: Type: text/plain, Size: 8653 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git j1939
head: 126ad77985046d48218f25ed6f18d06bf1fa78dd
commit: 126ad77985046d48218f25ed6f18d06bf1fa78dd [1/1] can-j1939: Import SAE J1939
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
git checkout 126ad77985046d48218f25ed6f18d06bf1fa78dd
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
net/can/j1939/socket.c: In function 'j1939sk_sendmsg':
>> net/can/j1939/socket.c:707:24: warning: passing argument 2 of 'sock_tx_timestamp' makes integer from pointer without a cast [-Wint-conversion]
sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
^
In file included from include/linux/can/skb.h:16:0,
from net/can/j1939/socket.c:22:
include/net/sock.h:2159:20: note: expected '__u16 {aka short unsigned int}' but argument is of type '__u8 * {aka unsigned char *}'
static inline void sock_tx_timestamp(const struct sock *sk, __u16 tsflags,
^~~~~~~~~~~~~~~~~
>> net/can/j1939/socket.c:707:2: error: too few arguments to function 'sock_tx_timestamp'
sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
^~~~~~~~~~~~~~~~~
In file included from include/linux/can/skb.h:16:0,
from net/can/j1939/socket.c:22:
include/net/sock.h:2159:20: note: declared here
static inline void sock_tx_timestamp(const struct sock *sk, __u16 tsflags,
^~~~~~~~~~~~~~~~~
coccinelle warnings: (new ones prefixed by >>)
>> net/can/j1939/socket.c:528:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.
--
>> net/can/j1939/socket.c:510:13-20: WARNING opportunity for memdup_user
Please review and possibly fold the followup patch.
vim +/sock_tx_timestamp +707 net/can/j1939/socket.c
504 switch (optname) {
505 case SO_J1939_FILTER:
506 if (optval) {
507 if (optlen % sizeof(*filters) != 0)
508 return -EINVAL;
509 count = optlen / sizeof(*filters);
> 510 filters = kmalloc(optlen, GFP_KERNEL);
511 if (!filters)
512 return -ENOMEM;
513 if (copy_from_user(filters, optval, optlen)) {
514 kfree(filters);
515 return -EFAULT;
516 }
517 } else {
518 filters = NULL;
519 count = 0;
520 }
521
522 spin_lock_bh(&j1939_socks_lock);
523 ofilters = jsk->filters;
524 jsk->filters = filters;
525 jsk->nfilters = count;
526 spin_unlock_bh(&j1939_socks_lock);
527 if (ofilters)
> 528 kfree(ofilters);
529 return 0;
530 case SO_J1939_PROMISC:
531 return j1939sk_setsockopt_flag(jsk, optval, optlen, PROMISC);
532 case SO_J1939_RECV_OWN:
533 return j1939sk_setsockopt_flag(jsk, optval, optlen, RECV_OWN);
534 case SO_J1939_SEND_PRIO:
535 if (optlen != sizeof(tmp))
536 return -EINVAL;
537 if (copy_from_user(&tmp, optval, optlen))
538 return -EFAULT;
539 if ((tmp < 0) || (tmp > 7))
540 return -EDOM;
541 if ((tmp < 2) && !capable(CAP_NET_ADMIN))
542 return -EPERM;
543 lock_sock(&jsk->sk);
544 jsk->sk.sk_priority = j1939_to_sk_priority(tmp);
545 release_sock(&jsk->sk);
546 return 0;
547 default:
548 return -ENOPROTOOPT;
549 }
550 }
551
552 static int j1939sk_getsockopt(struct socket *sock, int level, int optname,
553 char __user *optval, int __user *optlen)
554 {
555 struct sock *sk = sock->sk;
556 struct j1939_sock *jsk = j1939_sk(sk);
557 int ret, ulen;
558 /* set defaults for using 'int' properties */
559 int tmp = 0;
560 int len = sizeof(tmp);
561 void *val = &tmp;
562
563 if (level != SOL_CAN_J1939)
564 return -EINVAL;
565 if (get_user(ulen, optlen))
566 return -EFAULT;
567 if (ulen < 0)
568 return -EINVAL;
569
570 lock_sock(&jsk->sk);
571 switch (optname) {
572 case SO_J1939_PROMISC:
573 tmp = (jsk->state & PROMISC) ? 1 : 0;
574 break;
575 case SO_J1939_RECV_OWN:
576 tmp = (jsk->state & RECV_OWN) ? 1 : 0;
577 break;
578 case SO_J1939_SEND_PRIO:
579 tmp = j1939_prio(jsk->sk.sk_priority);
580 break;
581 default:
582 ret = -ENOPROTOOPT;
583 goto no_copy;
584 }
585
586 /*
587 * copy to user, based on 'len' & 'val'
588 * but most sockopt's are 'int' properties, and have 'len' & 'val'
589 * left unchanged, but instead modified 'tmp'
590 */
591 if (len > ulen)
592 ret = -EFAULT;
593 else if (put_user(len, optlen))
594 ret = -EFAULT;
595 else if (copy_to_user(optval, val, len))
596 ret = -EFAULT;
597 else
598 ret = 0;
599 no_copy:
600 release_sock(&jsk->sk);
601 return ret;
602 }
603
604 static int j1939sk_recvmsg(struct socket *sock, struct msghdr *msg,
605 size_t size, int flags)
606 {
607 struct sock *sk = sock->sk;
608 struct sk_buff *skb;
609 struct j1939_sk_buff_cb *skcb;
610 int ret = 0;
611
612 skb = skb_recv_datagram(sk, flags, 0, &ret);
613 if (!skb)
614 return ret;
615
616 if (size < skb->len)
617 msg->msg_flags |= MSG_TRUNC;
618 else
619 size = skb->len;
620
621 ret = memcpy_to_msg(msg, skb->data, size);
622 if (ret < 0) {
623 skb_free_datagram(sk, skb);
624 return ret;
625 }
626
627 skcb = (void *)skb->cb;
628 if (j1939_address_is_valid(skcb->dstaddr))
629 put_cmsg(msg, SOL_CAN_J1939, SCM_J1939_DEST_ADDR,
630 sizeof(skcb->dstaddr), &skcb->dstaddr);
631
632 if (skcb->dstname)
633 put_cmsg(msg, SOL_CAN_J1939, SCM_J1939_DEST_NAME,
634 sizeof(skcb->dstname), &skcb->dstname);
635
636 put_cmsg(msg, SOL_CAN_J1939, SCM_J1939_PRIO,
637 sizeof(skcb->priority), &skcb->priority);
638
639 if (msg->msg_name) {
640 struct sockaddr_can *paddr = msg->msg_name;
641
642 msg->msg_namelen = J1939_MIN_NAMELEN;
643 memset(msg->msg_name, 0, msg->msg_namelen);
644 paddr->can_family = AF_CAN;
645 paddr->can_ifindex = skb->skb_iif;
646 paddr->can_addr.j1939.name = skcb->srcname;
647 paddr->can_addr.j1939.addr = skcb->srcaddr;
648 paddr->can_addr.j1939.pgn = skcb->pgn;
649 }
650
651 sock_recv_ts_and_drops(msg, sk, skb);
652 msg->msg_flags |= skcb->msg_flags;
653 skb_free_datagram(sk, skb);
654
655 return size;
656 }
657
658 static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
659 {
660 struct sock *sk = sock->sk;
661 struct j1939_sock *jsk = j1939_sk(sk);
662 struct sockaddr_can *addr = msg->msg_name;
663 struct j1939_sk_buff_cb *skcb;
664 struct sk_buff *skb;
665 struct net_device *dev;
666 int ifindex;
667 int ret;
668
669 /* various socket state tests */
670 if (!(jsk->state & JSK_BOUND))
671 return -EBADFD;
672
673 ifindex = jsk->ifindex_started;
674 if (!ifindex)
675 return -EBADFD;
676
677 if (jsk->addr.sa == J1939_NO_ADDR && !jsk->addr.src)
678 /* no address assigned yet */
679 return -EBADFD;
680
681 /* deal with provided address info */
682 if (msg->msg_name) {
683 if (msg->msg_namelen < J1939_MIN_NAMELEN)
684 return -EINVAL;
685 if (addr->can_family != AF_CAN)
686 return -EINVAL;
687 if (addr->can_ifindex && (ifindex != addr->can_ifindex))
688 return -EBADFD;
689 }
690
691 dev = dev_get_by_index(&init_net, ifindex);
692 if (!dev)
693 return -ENXIO;
694
695 skb = sock_alloc_send_skb(sk, size + sizeof(struct can_frame) - sizeof(((struct can_frame*)NULL)->data) + sizeof(struct can_skb_priv),
696 msg->msg_flags & MSG_DONTWAIT, &ret);
697 if (!skb)
698 goto put_dev;
699
700 can_skb_reserve(skb);
701 can_skb_prv(skb)->ifindex = dev->ifindex;
702 skb_reserve(skb, offsetof(struct can_frame, data));
703
704 ret = memcpy_from_msg(skb_put(skb, size), msg, size);
705 if (ret < 0)
706 goto free_skb;
> 707 sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
708
709 skb->dev = dev;
710
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 55086 bytes --]
^ permalink raw reply
* [PATCH] can-j1939: fix ifnullfree.cocci warnings
From: kbuild test robot @ 2016-09-06 11:58 UTC (permalink / raw)
To: Kurt Van Dijck; +Cc: kbuild-all, linux-can, Marc Kleine-Budde
In-Reply-To: <201609061930.NHHial3C%fengguang.wu@intel.com>
net/can/j1939/socket.c:528:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.
NULL check before some freeing functions is not needed.
Based on checkpatch warning
"kfree(NULL) is safe this check is probably not required"
and kfreeaddr.cocci by Julia Lawall.
Generated by: scripts/coccinelle/free/ifnullfree.cocci
CC: Kurt Van Dijck <kurt.van.dijck@eia.be>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
socket.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -524,8 +524,7 @@ static int j1939sk_setsockopt(struct soc
jsk->filters = filters;
jsk->nfilters = count;
spin_unlock_bh(&j1939_socks_lock);
- if (ofilters)
- kfree(ofilters);
+ kfree(ofilters);
return 0;
case SO_J1939_PROMISC:
return j1939sk_setsockopt_flag(jsk, optval, optlen, PROMISC);
^ permalink raw reply
* j1939: j1939tp_txnext(): review switch (session->last_cmd)
From: Marc Kleine-Budde @ 2016-09-06 11:01 UTC (permalink / raw)
To: linux-can, David Jander, kurt.van.dijck
[-- Attachment #1.1: Type: text/plain, Size: 5569 bytes --]
Hello,
I'm currently reviewing Kurt's et al. j1939 patches and checkpatch
stumbled over this switch():
> /*
> * transmit function
> */
> static int j1939tp_txnext(struct session *session)
> {
> uint8_t dat[8];
> const uint8_t *tpdat;
> int ret, offset, pkt_done, pkt_end;
> unsigned int pkt, len, pdelay;
>
> memset(dat, 0xff, sizeof(dat));
> get_session(session); /* do not loose it */
>
> switch (session->last_cmd) {
> case 0:
> if (!j1939tp_im_transmitter(session->skb))
> break;
> dat[1] = (session->skb->len >> 0) & 0xff;
> dat[2] = (session->skb->len >> 8) & 0xff;
> dat[3] = session->pkt.total;
> if (session->extd) {
> dat[0] = etp_cmd_rts;
> dat[1] = (session->skb->len >> 0) & 0xff;
> dat[2] = (session->skb->len >> 8) & 0xff;
> dat[3] = (session->skb->len >> 16) & 0xff;
> dat[4] = (session->skb->len >> 24) & 0xff;
> } else if (j1939cb_is_broadcast(session->cb)) {
> dat[0] = tp_cmd_bam;
> /* fake cts for broadcast */
> session->pkt.tx = 0;
> } else {
> dat[0] = tp_cmd_rts;
> dat[4] = dat[3];
> }
> if (dat[0] == session->last_txcmd)
> /* done already */
> break;
> ret = j1939tp_tx_ctl(session, 0, dat);
> if (ret < 0)
> goto failed;
> session->last_txcmd = dat[0];
> /* must lock? */
> if (tp_cmd_bam == dat[0])
> j1939tp_schedule_txtimer(session, 50);
> j1939tp_set_rxtimeout(session, 1250);
> break;
> case tp_cmd_rts:
> case etp_cmd_rts:
> if (!j1939tp_im_receiver(session->skb))
> break;
> tx_cts:
> ret = 0;
> len = session->pkt.total - session->pkt.done;
> len = min(max(len, session->pkt.block), block ?: 255);
>
> if (session->extd) {
> pkt = session->pkt.done+1;
> dat[0] = etp_cmd_cts;
> dat[1] = len;
> dat[2] = (pkt >> 0) & 0xff;
> dat[3] = (pkt >> 8) & 0xff;
> dat[4] = (pkt >> 16) & 0xff;
> } else {
> dat[0] = tp_cmd_cts;
> dat[1] = len;
> dat[2] = session->pkt.done+1;
> }
> if (dat[0] == session->last_txcmd)
> /* done already */
> break;
> ret = j1939tp_tx_ctl(session, 1, dat);
> if (ret < 0)
> goto failed;
> if (len)
> /* only mark cts done when len is set */
> session->last_txcmd = dat[0];
> j1939tp_set_rxtimeout(session, 1250);
> break;
> case etp_cmd_cts:
> if (j1939tp_im_transmitter(session->skb) && session->extd &&
> (etp_cmd_dpo != session->last_txcmd)) {
> /* do dpo */
> dat[0] = etp_cmd_dpo;
> session->pkt.dpo = session->pkt.done;
> pkt = session->pkt.dpo;
> dat[1] = session->pkt.last - session->pkt.done;
> dat[2] = (pkt >> 0) & 0xff;
> dat[3] = (pkt >> 8) & 0xff;
> dat[4] = (pkt >> 16) & 0xff;
> ret = j1939tp_tx_ctl(session, 0, dat);
> if (ret < 0)
> goto failed;
> session->last_txcmd = dat[0];
> j1939tp_set_rxtimeout(session, 1250);
> session->pkt.tx = session->pkt.done;
> }
> case tp_cmd_cts:
> case 0xff: /* did some data */
> case etp_cmd_dpo:
> if ((session->extd || !j1939cb_is_broadcast(session->cb)) &&
> j1939tp_im_receiver(session->skb)) {
> if (session->pkt.done >= session->pkt.total) {
> if (session->extd) {
> dat[0] = etp_cmd_eof;
> dat[1] = session->skb->len >> 0;
> dat[2] = session->skb->len >> 8;
> dat[3] = session->skb->len >> 16;
> dat[4] = session->skb->len >> 24;
> } else {
> dat[0] = tp_cmd_eof;
> dat[1] = session->skb->len;
> dat[2] = session->skb->len >> 8;
> dat[3] = session->pkt.total;
> }
> if (dat[0] == session->last_txcmd)
> /* done already */
> break;
> ret = j1939tp_tx_ctl(session, 1, dat);
> if (ret < 0)
> goto failed;
> session->last_txcmd = dat[0];
> j1939tp_set_rxtimeout(session, 1250);
> /* wait for the EOF packet to come in */
> break;
> } else if (session->pkt.done >= session->pkt.last) {
> session->last_txcmd = 0;
> goto tx_cts;
> }
> }
> case tp_cmd_bam:
> if (!j1939tp_im_transmitter(session->skb))
> break;
> tpdat = session->skb->data;
> ret = 0;
> pkt_done = 0;
> pkt_end = (!session->extd && j1939cb_is_broadcast(session->cb))
> ? session->pkt.total : session->pkt.last;
>
> while (session->pkt.tx < pkt_end) {
> dat[0] = session->pkt.tx - session->pkt.dpo+1;
> offset = session->pkt.tx * 7;
> len = session->skb->len - offset;
> if (len > 7)
> len = 7;
> memcpy(&dat[1], &tpdat[offset], len);
> ret = j1939tp_tx_dat(session->skb, session->extd,
> dat, len+1);
> if (ret < 0)
> break;
> session->last_txcmd = 0xff;
> ++pkt_done;
> ++session->pkt.tx;
> pdelay = j1939cb_is_broadcast(session->cb) ? 50 :
> packet_delay;
> if ((session->pkt.tx < session->pkt.total) && pdelay) {
> j1939tp_schedule_txtimer(session, pdelay);
> break;
> }
> }
> if (pkt_done)
> j1939tp_set_rxtimeout(session, 250);
> if (ret)
> goto failed;
> break;
> }
> put_session(session);
> return 0;
> failed:
> put_session(session);
> return ret;
> }
The question is, are there missing any break statements? If not, I'm
going to add fall-though comments.
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: 455 bytes --]
^ permalink raw reply
* [PATCH RESEND v3] can: flexcan: fix resume function
From: Fabio Estevam @ 2016-09-05 22:15 UTC (permalink / raw)
To: mkl; +Cc: wg, linux-can, Fabio Estevam
From: Fabio Estevam <fabio.estevam@nxp.com>
On a imx6ul-pico board the following error is seen during system suspend:
dpm_run_callback(): platform_pm_resume+0x0/0x54 returns -110
PM: Device 2090000.flexcan failed to resume: error -110
The reason for this suspend error is because when the CAN interface is not
active the clocks are disabled and then flexcan_chip_enable() will
always fail due to a timeout error.
In order to fix this issue, only call flexcan_chip_enable/disable()
when the CAN interface is active.
Based on a patch from Dong Aisheng in the NXP kernel.
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
Changes since v2:
- Move flexcan_chip_disable() to be called as the first function in
flexcan_suspen() to keep the original order.
Changes since v1:
- Change Subject: 'resume' instead of 'suspend'
drivers/net/can/flexcan.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 41c0fc9..16f7cad 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1268,11 +1268,10 @@ static int __maybe_unused flexcan_suspend(struct device *device)
struct flexcan_priv *priv = netdev_priv(dev);
int err;
- err = flexcan_chip_disable(priv);
- if (err)
- return err;
-
if (netif_running(dev)) {
+ err = flexcan_chip_disable(priv);
+ if (err)
+ return err;
netif_stop_queue(dev);
netif_device_detach(dev);
}
@@ -1285,13 +1284,17 @@ static int __maybe_unused flexcan_resume(struct device *device)
{
struct net_device *dev = dev_get_drvdata(device);
struct flexcan_priv *priv = netdev_priv(dev);
+ int err;
priv->can.state = CAN_STATE_ERROR_ACTIVE;
if (netif_running(dev)) {
netif_device_attach(dev);
netif_start_queue(dev);
+ err = flexcan_chip_enable(priv);
+ if (err)
+ return err;
}
- return flexcan_chip_enable(priv);
+ return 0;
}
static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend, flexcan_resume);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] can: sja1000: Optimise register accesses
From: Oliver Hartkopp @ 2016-09-05 19:56 UTC (permalink / raw)
To: Nikita Edward Baruzdin, linux-can
In-Reply-To: <20160905175852.1513-2-nebaruzdin@gmail.com>
Hello Nikita,
On 09/05/2016 07:58 PM, Nikita Edward Baruzdin wrote:
> Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
> instead of consecutive ioread8()/iowrite8() calls seems reasonable.
I definitely like this patch.
> Thus, this patch introduces plx_pci_read_reg_rep() and
> plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
> register accesses as much as possible. The functions are then used to
> read/write CAN ID and payload data to improve driver performance.
But there is something more to do as you currently brake all sja1000
drivers except plx_pci ;-)
You need to take care of drivers that do not support
priv->[read|write]_reg_rep().
So that other (pci) drivers can follow at will after testing.
> Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
> ---
> drivers/net/can/sja1000/plx_pci.c | 42 +++++++++++++++++++++++++++++++++++++++
> drivers/net/can/sja1000/sja1000.c | 30 +++++++++++-----------------
> drivers/net/can/sja1000/sja1000.h | 4 ++++
> 3 files changed, 58 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
> index 3eb7430..7aaf6b9 100644
> --- a/drivers/net/can/sja1000/plx_pci.c
> +++ b/drivers/net/can/sja1000/plx_pci.c
> @@ -371,6 +371,46 @@ static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 val)
> iowrite8(val, priv->reg_base + port);
> }
>
> +static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
> + void *buffer, unsigned long count)
> +{
> + u8 *p = buffer;
> +
> + while (count >= 4) {
> + *(u32 *)p = ioread32(priv->reg_base + reg);
> + p += 4;
> + reg += 4;
> + count -= 4;
> + }
> + if (count & 2) {
> + *(u16 *)p = ioread16(priv->reg_base + reg);
> + p += 2;
> + reg += 2;
> + }
> + if (count & 1)
> + *p = ioread8(priv->reg_base + reg);
> +}
> +
> +static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
> + const void *buffer, unsigned long count)
> +{
> + const u8 *p = buffer;
> +
> + while (count >= 4) {
> + iowrite32(*(u32 *)p, priv->reg_base + reg);
> + p += 4;
> + reg += 4;
> + count -= 4;
> + }
> + if (count & 2) {
> + iowrite16(*(u16 *)p, priv->reg_base + reg);
> + p += 2;
> + reg += 2;
> + }
> + if (count & 1)
> + iowrite8(*p, priv->reg_base + reg);
> +}
> +
> /*
> * Check if a CAN controller is present at the specified location
> * by trying to switch 'em from the Basic mode into the PeliCAN mode.
> @@ -626,6 +666,8 @@ static int plx_pci_add_card(struct pci_dev *pdev,
> priv->reg_base = addr + cm->offset;
> priv->read_reg = plx_pci_read_reg;
> priv->write_reg = plx_pci_write_reg;
> + priv->read_reg_rep = plx_pci_read_reg_rep;
> + priv->write_reg_rep = plx_pci_write_reg_rep;
>
> /* Check if channel is present */
> if (plx_pci_check_sja1000(priv)) {
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 9f10779..b26173e 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -288,7 +288,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
> canid_t id;
> uint8_t dreg;
> u8 cmd_reg_val = 0x00;
> - int i;
This 'i' needs to be preserved, see below.
> + u8 id_buf[4];
>
> if (can_dropped_invalid_skb(dev, skb))
> return NETDEV_TX_OK;
> @@ -305,19 +305,16 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
> fi |= SJA1000_FI_FF;
> dreg = SJA1000_EFF_BUF;
> priv->write_reg(priv, SJA1000_FI, fi);
> - priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
> - priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
> - priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
> - priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
Has to be
if (priv->write_reg_rep) {
*(u32 *)id_buf = cpu_to_be32(id << 3);
priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
} else {
priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
}
at all occurrences of priv->[read|write]_reg_rep().
> } else {
> dreg = SJA1000_SFF_BUF;
> priv->write_reg(priv, SJA1000_FI, fi);
> - priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
> - priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
> + *(u16 *)id_buf = cpu_to_be16(id << 5);
> + priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 2);
Like here.
> }
>
> - for (i = 0; i < dlc; i++)
> - priv->write_reg(priv, dreg++, cf->data[i]);
> + priv->write_reg_rep(priv, dreg, cf->data, dlc);
dito
>
> can_put_echo_skb(skb, dev, 0);
>
> @@ -343,7 +340,7 @@ static void sja1000_rx(struct net_device *dev)
> uint8_t fi;
> uint8_t dreg;
> canid_t id;
> - int i;
> + u8 id_buf[4];
>
> /* create zero'ed CAN frame buffer */
> skb = alloc_can_skb(dev, &cf);
> @@ -355,24 +352,21 @@ static void sja1000_rx(struct net_device *dev)
> if (fi & SJA1000_FI_FF) {
> /* extended frame format (EFF) */
> dreg = SJA1000_EFF_BUF;
> - id = (priv->read_reg(priv, SJA1000_ID1) << 21)
> - | (priv->read_reg(priv, SJA1000_ID2) << 13)
> - | (priv->read_reg(priv, SJA1000_ID3) << 5)
> - | (priv->read_reg(priv, SJA1000_ID4) >> 3);
> + priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 4);
> + id = be32_to_cpu(*(u32 *)id_buf) >> 3;
here
> id |= CAN_EFF_FLAG;
> } else {
> /* standard frame format (SFF) */
> dreg = SJA1000_SFF_BUF;
> - id = (priv->read_reg(priv, SJA1000_ID1) << 3)
> - | (priv->read_reg(priv, SJA1000_ID2) >> 5);
> + priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 2);
> + id = be16_to_cpu(*(u16 *)id_buf) >> 5;
here
> }
>
> cf->can_dlc = get_can_dlc(fi & 0x0F);
> if (fi & SJA1000_FI_RTR) {
> id |= CAN_RTR_FLAG;
> } else {
> - for (i = 0; i < cf->can_dlc; i++)
> - cf->data[i] = priv->read_reg(priv, dreg++);
> + priv->read_reg_rep(priv, dreg, cf->data, cf->can_dlc);
and here.
Additionally:
You always take care of the CPU endian when reading writing CAN
identifiers. Why is it ok to use read_reg_rep() for the CAN data content
without any beXX_to_cpu() conversion?
Regards,
Oliver
> }
>
> cf->can_id = id;
> diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
> index 9d46398..887045a 100644
> --- a/drivers/net/can/sja1000/sja1000.h
> +++ b/drivers/net/can/sja1000/sja1000.h
> @@ -157,6 +157,10 @@ struct sja1000_priv {
> /* the lower-layer is responsible for appropriate locking */
> u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
> void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
> + void (*read_reg_rep) (const struct sja1000_priv *priv, int reg,
> + void *buffer, unsigned long count);
> + void (*write_reg_rep) (const struct sja1000_priv *priv, int reg,
> + const void *buffer, unsigned long count);
> void (*pre_irq) (const struct sja1000_priv *priv);
> void (*post_irq) (const struct sja1000_priv *priv);
>
>
^ permalink raw reply
* [PATCH] can: sja1000: Optimise register accesses
From: Nikita Edward Baruzdin @ 2016-09-05 17:58 UTC (permalink / raw)
To: linux-can
In-Reply-To: <20160905175852.1513-1-nebaruzdin@gmail.com>
Since PCI bus width is at least 32 bits, using ioread32()/iowrite32()
instead of consecutive ioread8()/iowrite8() calls seems reasonable.
Thus, this patch introduces plx_pci_read_reg_rep() and
plx_pci_write_reg_rep() functions that use ioread32()/iowrite32() for
register accesses as much as possible. The functions are then used to
read/write CAN ID and payload data to improve driver performance.
Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
---
drivers/net/can/sja1000/plx_pci.c | 42 +++++++++++++++++++++++++++++++++++++++
drivers/net/can/sja1000/sja1000.c | 30 +++++++++++-----------------
drivers/net/can/sja1000/sja1000.h | 4 ++++
3 files changed, 58 insertions(+), 18 deletions(-)
diff --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c
index 3eb7430..7aaf6b9 100644
--- a/drivers/net/can/sja1000/plx_pci.c
+++ b/drivers/net/can/sja1000/plx_pci.c
@@ -371,6 +371,46 @@ static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 val)
iowrite8(val, priv->reg_base + port);
}
+static void plx_pci_read_reg_rep(const struct sja1000_priv *priv, int reg,
+ void *buffer, unsigned long count)
+{
+ u8 *p = buffer;
+
+ while (count >= 4) {
+ *(u32 *)p = ioread32(priv->reg_base + reg);
+ p += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ *(u16 *)p = ioread16(priv->reg_base + reg);
+ p += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ *p = ioread8(priv->reg_base + reg);
+}
+
+static void plx_pci_write_reg_rep(const struct sja1000_priv *priv, int reg,
+ const void *buffer, unsigned long count)
+{
+ const u8 *p = buffer;
+
+ while (count >= 4) {
+ iowrite32(*(u32 *)p, priv->reg_base + reg);
+ p += 4;
+ reg += 4;
+ count -= 4;
+ }
+ if (count & 2) {
+ iowrite16(*(u16 *)p, priv->reg_base + reg);
+ p += 2;
+ reg += 2;
+ }
+ if (count & 1)
+ iowrite8(*p, priv->reg_base + reg);
+}
+
/*
* Check if a CAN controller is present at the specified location
* by trying to switch 'em from the Basic mode into the PeliCAN mode.
@@ -626,6 +666,8 @@ static int plx_pci_add_card(struct pci_dev *pdev,
priv->reg_base = addr + cm->offset;
priv->read_reg = plx_pci_read_reg;
priv->write_reg = plx_pci_write_reg;
+ priv->read_reg_rep = plx_pci_read_reg_rep;
+ priv->write_reg_rep = plx_pci_write_reg_rep;
/* Check if channel is present */
if (plx_pci_check_sja1000(priv)) {
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 9f10779..b26173e 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -288,7 +288,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
canid_t id;
uint8_t dreg;
u8 cmd_reg_val = 0x00;
- int i;
+ u8 id_buf[4];
if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
@@ -305,19 +305,16 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
fi |= SJA1000_FI_FF;
dreg = SJA1000_EFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x1fe00000) >> 21);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x001fe000) >> 13);
- priv->write_reg(priv, SJA1000_ID3, (id & 0x00001fe0) >> 5);
- priv->write_reg(priv, SJA1000_ID4, (id & 0x0000001f) << 3);
+ *(u32 *)id_buf = cpu_to_be32(id << 3);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 4);
} else {
dreg = SJA1000_SFF_BUF;
priv->write_reg(priv, SJA1000_FI, fi);
- priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
- priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
+ *(u16 *)id_buf = cpu_to_be16(id << 5);
+ priv->write_reg_rep(priv, SJA1000_ID1, id_buf, 2);
}
- for (i = 0; i < dlc; i++)
- priv->write_reg(priv, dreg++, cf->data[i]);
+ priv->write_reg_rep(priv, dreg, cf->data, dlc);
can_put_echo_skb(skb, dev, 0);
@@ -343,7 +340,7 @@ static void sja1000_rx(struct net_device *dev)
uint8_t fi;
uint8_t dreg;
canid_t id;
- int i;
+ u8 id_buf[4];
/* create zero'ed CAN frame buffer */
skb = alloc_can_skb(dev, &cf);
@@ -355,24 +352,21 @@ static void sja1000_rx(struct net_device *dev)
if (fi & SJA1000_FI_FF) {
/* extended frame format (EFF) */
dreg = SJA1000_EFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 21)
- | (priv->read_reg(priv, SJA1000_ID2) << 13)
- | (priv->read_reg(priv, SJA1000_ID3) << 5)
- | (priv->read_reg(priv, SJA1000_ID4) >> 3);
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 4);
+ id = be32_to_cpu(*(u32 *)id_buf) >> 3;
id |= CAN_EFF_FLAG;
} else {
/* standard frame format (SFF) */
dreg = SJA1000_SFF_BUF;
- id = (priv->read_reg(priv, SJA1000_ID1) << 3)
- | (priv->read_reg(priv, SJA1000_ID2) >> 5);
+ priv->read_reg_rep(priv, SJA1000_ID1, id_buf, 2);
+ id = be16_to_cpu(*(u16 *)id_buf) >> 5;
}
cf->can_dlc = get_can_dlc(fi & 0x0F);
if (fi & SJA1000_FI_RTR) {
id |= CAN_RTR_FLAG;
} else {
- for (i = 0; i < cf->can_dlc; i++)
- cf->data[i] = priv->read_reg(priv, dreg++);
+ priv->read_reg_rep(priv, dreg, cf->data, cf->can_dlc);
}
cf->can_id = id;
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398..887045a 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -157,6 +157,10 @@ struct sja1000_priv {
/* the lower-layer is responsible for appropriate locking */
u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
+ void (*read_reg_rep) (const struct sja1000_priv *priv, int reg,
+ void *buffer, unsigned long count);
+ void (*write_reg_rep) (const struct sja1000_priv *priv, int reg,
+ const void *buffer, unsigned long count);
void (*pre_irq) (const struct sja1000_priv *priv);
void (*post_irq) (const struct sja1000_priv *priv);
--
2.9.3
^ permalink raw reply related
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