Re: [PATCH net-next v2 0/5] net: pch: fix and a few cleanups
On Thu, Mar 25, 2021 at 07:34:07PM +0200, Andy Shevchenko wrote: > The series provides one fix (patch 1) for GPIO to be able to wait for > the GPIO driver to appear. This is separated from the conversion to > the GPIO descriptors (patch 2) in order to have a possibility for > backporting. Patches 3 and 4 fix a minor warnings from Sparse while > moving to a new APIs. Patch 5 is MODULE_VERSION() clean up. > > Tested on Intel Minnowboard (v1). Guys, it has been already the report from kbuild bot (sparse warnings), which should be fixed by this series (at least partially if not completely). Please, apply this as soon as it's possible. Or tell me what's wrong with the series. Thanks! > Since v2: > - added a few cleanups on top of the fix > > Andy Shevchenko (5): > net: pch_gbe: Propagate error from devm_gpio_request_one() > net: pch_gbe: Convert to use GPIO descriptors > net: pch_gbe: use readx_poll_timeout_atomic() variant > net: pch_gbe: Use proper accessors to BE data in pch_ptp_match() > net: pch_gbe: remove unneeded MODULE_VERSION() call > > .../net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 2 - > .../oki-semi/pch_gbe/pch_gbe_ethtool.c| 2 + > .../ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 103 +- > 3 files changed, 54 insertions(+), 53 deletions(-) > > -- > 2.30.2 > -- With Best Regards, Andy Shevchenko
[PATCH v2 1/1] kernel.h: Split out panic and oops helpers
kernel.h is being used as a dump for all kinds of stuff for a long time.
Here is the attempt to start cleaning it up by splitting out panic and
oops helpers.
There are several purposes of doing this:
- dropping dependency in bug.h
- dropping a loop by moving out panic_notifier.h
- unload kernel.h from something which has its own domain
At the same time convert users tree-wide to use new headers, although
for the time being include new header back to kernel.h to avoid twisted
indirected includes for existing users.
Signed-off-by: Andy Shevchenko
Reviewed-by: Bjorn Andersson
Acked-by: Mike Rapoport
Acked-by: Corey Minyard
Acked-by: Christian Brauner
Acked-by: Arnd Bergmann
Acked-by: Kees Cook
Acked-by: Wei Liu
Acked-by: Rasmus Villemoes
Signed-off-by: Andrew Morton
---
v2:
- fixed all errors with allmodconfig on x86_64 (Andrew)
- checked with allyesconfig on x86_64
- additionally grepped source code for panic notifier list usage
and converted all users
- elaborated commit message (Luis)
- collected given tags (incl. Andrew's SoB, see below)
I added Andrew's SoB since part of the fixes I took from him. Andrew,
feel free to amend or tell me how you want me to do.
arch/alpha/kernel/setup.c | 2 +-
arch/arm64/kernel/setup.c | 1 +
arch/mips/kernel/relocate.c | 1 +
arch/mips/sgi-ip22/ip22-reset.c | 1 +
arch/mips/sgi-ip32/ip32-reset.c | 1 +
arch/parisc/kernel/pdc_chassis.c | 1 +
arch/powerpc/kernel/setup-common.c| 1 +
arch/s390/kernel/ipl.c| 1 +
arch/sparc/kernel/sstate.c| 1 +
arch/um/drivers/mconsole_kern.c | 1 +
arch/um/kernel/um_arch.c | 1 +
arch/x86/include/asm/desc.h | 1 +
arch/x86/kernel/cpu/mshyperv.c| 1 +
arch/x86/kernel/setup.c | 1 +
arch/x86/purgatory/purgatory.c| 2 +
arch/x86/xen/enlighten.c | 1 +
arch/xtensa/platforms/iss/setup.c | 1 +
drivers/bus/brcmstb_gisb.c| 1 +
drivers/char/ipmi/ipmi_msghandler.c | 1 +
drivers/clk/analogbits/wrpll-cln28hpc.c | 4 +
drivers/edac/altera_edac.c| 1 +
drivers/firmware/google/gsmi.c| 1 +
drivers/hv/vmbus_drv.c| 1 +
.../hwtracing/coresight/coresight-cpu-debug.c | 1 +
drivers/leds/trigger/ledtrig-activity.c | 1 +
drivers/leds/trigger/ledtrig-heartbeat.c | 1 +
drivers/leds/trigger/ledtrig-panic.c | 1 +
drivers/misc/bcm-vk/bcm_vk_dev.c | 1 +
drivers/misc/ibmasm/heartbeat.c | 1 +
drivers/misc/pvpanic/pvpanic.c| 1 +
drivers/net/ipa/ipa_smp2p.c | 1 +
drivers/parisc/power.c| 1 +
drivers/power/reset/ltc2952-poweroff.c| 1 +
drivers/remoteproc/remoteproc_core.c | 1 +
drivers/s390/char/con3215.c | 1 +
drivers/s390/char/con3270.c | 1 +
drivers/s390/char/sclp.c | 1 +
drivers/s390/char/sclp_con.c | 1 +
drivers/s390/char/sclp_vt220.c| 1 +
drivers/s390/char/zcore.c | 1 +
drivers/soc/bcm/brcmstb/pm/pm-arm.c | 1 +
drivers/staging/olpc_dcon/olpc_dcon.c | 1 +
drivers/video/fbdev/hyperv_fb.c | 1 +
include/asm-generic/bug.h | 3 +-
include/linux/kernel.h| 84 +---
include/linux/panic.h | 98 +++
include/linux/panic_notifier.h| 12 +++
kernel/hung_task.c| 1 +
kernel/kexec_core.c | 1 +
kernel/panic.c| 1 +
kernel/rcu/tree.c | 2 +
kernel/sysctl.c | 1 +
kernel/trace/trace.c | 1 +
53 files changed, 167 insertions(+), 85 deletions(-)
create mode 100644 include/linux/panic.h
create mode 100644 include/linux/panic_notifier.h
diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index 03dda3beb3bd..5d1296534682 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -46,7 +47,6 @@
#include
#include
-extern struct atomic_notifier_head panic_notifier_list;
static int alpha_panic_event(struct notifier_block *, unsigned long, void *);
static struct notifier_block alpha_panic_block = {
alpha_panic_event,
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 61845c0821d9..787bc0f601b3 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -23,6 +23,7 @@
#include
#include
Re: [PATCH net-next v2 0/5] net: pch: fix and a few cleanups
On Thu, Apr 08, 2021 at 09:57:12AM +, Flavio Suligoi wrote: > > > > On Thu, Mar 25, 2021 at 07:34:07PM +0200, Andy Shevchenko wrote: > > > > > The series provides one fix (patch 1) for GPIO to be able to wait for > > > > > the GPIO driver to appear. This is separated from the conversion to > > > > > the GPIO descriptors (patch 2) in order to have a possibility for > > > > > backporting. Patches 3 and 4 fix a minor warnings from Sparse while > > > > > moving to a new APIs. Patch 5 is MODULE_VERSION() clean up. > > > > > > > > > > Tested on Intel Minnowboard (v1). > > > > > > > > Anything should I do here? > > > > > > it's ok for me > > > > Thanks! > > Who may apply them? > > I used your patches on kernel net-next 5.12.0-rc2, on a board with an > Intel(R) Atom(TM) CPU E640 @ 1.00GHz and an EG20T PCH. > I used the built-in OKI gigabit ethernet controller: > > 02:00.1 Ethernet controller: Intel Corporation Platform Controller Hub EG20T > Gigabit Ethernet Controller (rev 02) > Kernel driver in use: pch_gbe > > with a simple iperf test and all works fine: > I hope this can help you. > Tested-by: Flavio Suligoi Thank you, Flavio, very much! Jesse, Jakub, David. can this be applied, please? -- With Best Regards, Andy Shevchenko
Re: [PATCH net-next v2 0/5] net: pch: fix and a few cleanups
On Tue, Mar 30, 2021 at 07:46:58AM +, Flavio Suligoi wrote: > Hi Andy, > ... > > On Thu, Mar 25, 2021 at 07:34:07PM +0200, Andy Shevchenko wrote: > > > The series provides one fix (patch 1) for GPIO to be able to wait for > > > the GPIO driver to appear. This is separated from the conversion to > > > the GPIO descriptors (patch 2) in order to have a possibility for > > > backporting. Patches 3 and 4 fix a minor warnings from Sparse while > > > moving to a new APIs. Patch 5 is MODULE_VERSION() clean up. > > > > > > Tested on Intel Minnowboard (v1). > > > > Anything should I do here? > > it's ok for me Thanks! Who may apply them? -- With Best Regards, Andy Shevchenko
[PATCH v2 1/1] time64.h: Consolidated PSEC_PER_SEC definition
We have currently three users of the PSEC_PER_SEC each of them defining it
individually. Instead, move it to time64.h to be available for everyone.
There is a new user coming with the same constant in use. It will also
make its life easier.
Signed-off-by: Andy Shevchenko
Acked-by: Heiko Stuebner
---
v2: added tag (Heiko), rebased on top of newest rc
Since it touches PHY stuff, I assume that the PHY tree is the best
to suck this.
drivers/net/ethernet/mscc/ocelot_ptp.c | 2 ++
drivers/phy/phy-core-mipi-dphy.c | 2 --
drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c | 8
include/soc/mscc/ocelot_ptp.h| 2 --
include/vdso/time64.h| 1 +
5 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_ptp.c
b/drivers/net/ethernet/mscc/ocelot_ptp.c
index a33ab315cc6b..87ad2137ba06 100644
--- a/drivers/net/ethernet/mscc/ocelot_ptp.c
+++ b/drivers/net/ethernet/mscc/ocelot_ptp.c
@@ -4,6 +4,8 @@
* Copyright (c) 2017 Microsemi Corporation
* Copyright 2020 NXP
*/
+#include
+
#include
#include
#include
diff --git a/drivers/phy/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
index 14e0551cd319..77fe65367ce5 100644
--- a/drivers/phy/phy-core-mipi-dphy.c
+++ b/drivers/phy/phy-core-mipi-dphy.c
@@ -12,8 +12,6 @@
#include
#include
-#define PSEC_PER_SEC 1LL
-
/*
* Minimum D-PHY timings based on MIPI D-PHY specification. Derived
* from the valid ranges specified in Section 6.9, Table 14, Page 41
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
index 8af8c6c5cc02..347dc79a18c1 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
@@ -11,16 +11,16 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
+#include
+
#include
#include
-#include
-#include
-
-#define PSEC_PER_SEC 1LL
#define UPDATE(x, h, l)(((x) << (l)) & GENMASK((h), (l)))
diff --git a/include/soc/mscc/ocelot_ptp.h b/include/soc/mscc/ocelot_ptp.h
index 6a7388fa7cc5..ded497d72bdb 100644
--- a/include/soc/mscc/ocelot_ptp.h
+++ b/include/soc/mscc/ocelot_ptp.h
@@ -37,8 +37,6 @@ enum {
#define PTP_CFG_MISC_PTP_ENBIT(2)
-#define PSEC_PER_SEC 1LL
-
#define PTP_CFG_CLK_ADJ_CFG_ENABIT(0)
#define PTP_CFG_CLK_ADJ_CFG_DIRBIT(1)
diff --git a/include/vdso/time64.h b/include/vdso/time64.h
index 9d43c3f5e89d..b40cfa2aa33c 100644
--- a/include/vdso/time64.h
+++ b/include/vdso/time64.h
@@ -9,6 +9,7 @@
#define NSEC_PER_MSEC 100L
#define USEC_PER_SEC 100L
#define NSEC_PER_SEC 10L
+#define PSEC_PER_SEC 1LL
#define FSEC_PER_SEC 1000LL
#endif /* __VDSO_TIME64_H */
--
2.30.2
[PATCH net-next v1 1/1] stmmac: intel: Drop duplicate ID in the list of PCI device IDs
The PCI device IDs are defined with a prefix PCI_DEVICE_ID.
There is no need to repeat the ID part at the end of each definition.
Signed-off-by: Andy Shevchenko
---
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 60 +--
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 3d9a57043af2..7f0ce373a63d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -1053,41 +1053,41 @@ static int __maybe_unused intel_eth_pci_resume(struct
device *dev)
static SIMPLE_DEV_PM_OPS(intel_eth_pm_ops, intel_eth_pci_suspend,
intel_eth_pci_resume);
-#define PCI_DEVICE_ID_INTEL_QUARK_ID 0x0937
-#define PCI_DEVICE_ID_INTEL_EHL_RGMII1G_ID 0x4b30
-#define PCI_DEVICE_ID_INTEL_EHL_SGMII1G_ID 0x4b31
-#define PCI_DEVICE_ID_INTEL_EHL_SGMII2G5_ID0x4b32
+#define PCI_DEVICE_ID_INTEL_QUARK 0x0937
+#define PCI_DEVICE_ID_INTEL_EHL_RGMII1G0x4b30
+#define PCI_DEVICE_ID_INTEL_EHL_SGMII1G0x4b31
+#define PCI_DEVICE_ID_INTEL_EHL_SGMII2G5 0x4b32
/* Intel(R) Programmable Services Engine (Intel(R) PSE) consist of 2 MAC
* which are named PSE0 and PSE1
*/
-#define PCI_DEVICE_ID_INTEL_EHL_PSE0_RGMII1G_ID0x4ba0
-#define PCI_DEVICE_ID_INTEL_EHL_PSE0_SGMII1G_ID0x4ba1
-#define PCI_DEVICE_ID_INTEL_EHL_PSE0_SGMII2G5_ID 0x4ba2
-#define PCI_DEVICE_ID_INTEL_EHL_PSE1_RGMII1G_ID0x4bb0
-#define PCI_DEVICE_ID_INTEL_EHL_PSE1_SGMII1G_ID0x4bb1
-#define PCI_DEVICE_ID_INTEL_EHL_PSE1_SGMII2G5_ID 0x4bb2
-#define PCI_DEVICE_ID_INTEL_TGLH_SGMII1G_0_ID 0x43ac
-#define PCI_DEVICE_ID_INTEL_TGLH_SGMII1G_1_ID 0x43a2
-#define PCI_DEVICE_ID_INTEL_TGL_SGMII1G_ID 0xa0ac
-#define PCI_DEVICE_ID_INTEL_ADLS_SGMII1G_0_ID 0x7aac
-#define PCI_DEVICE_ID_INTEL_ADLS_SGMII1G_1_ID 0x7aad
+#define PCI_DEVICE_ID_INTEL_EHL_PSE0_RGMII1G 0x4ba0
+#define PCI_DEVICE_ID_INTEL_EHL_PSE0_SGMII1G 0x4ba1
+#define PCI_DEVICE_ID_INTEL_EHL_PSE0_SGMII2G5 0x4ba2
+#define PCI_DEVICE_ID_INTEL_EHL_PSE1_RGMII1G 0x4bb0
+#define PCI_DEVICE_ID_INTEL_EHL_PSE1_SGMII1G 0x4bb1
+#define PCI_DEVICE_ID_INTEL_EHL_PSE1_SGMII2G5 0x4bb2
+#define PCI_DEVICE_ID_INTEL_TGLH_SGMII1G_0 0x43ac
+#define PCI_DEVICE_ID_INTEL_TGLH_SGMII1G_1 0x43a2
+#define PCI_DEVICE_ID_INTEL_TGL_SGMII1G0xa0ac
+#define PCI_DEVICE_ID_INTEL_ADLS_SGMII1G_0 0x7aac
+#define PCI_DEVICE_ID_INTEL_ADLS_SGMII1G_1 0x7aad
static const struct pci_device_id intel_eth_pci_id_table[] = {
- { PCI_DEVICE_DATA(INTEL, QUARK_ID, &quark_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_RGMII1G_ID, &ehl_rgmii1g_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_SGMII1G_ID, &ehl_sgmii1g_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_SGMII2G5_ID, &ehl_sgmii1g_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_PSE0_RGMII1G_ID, &ehl_pse0_rgmii1g_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_PSE0_SGMII1G_ID, &ehl_pse0_sgmii1g_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_PSE0_SGMII2G5_ID, &ehl_pse0_sgmii1g_info)
},
- { PCI_DEVICE_DATA(INTEL, EHL_PSE1_RGMII1G_ID, &ehl_pse1_rgmii1g_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_PSE1_SGMII1G_ID, &ehl_pse1_sgmii1g_info) },
- { PCI_DEVICE_DATA(INTEL, EHL_PSE1_SGMII2G5_ID, &ehl_pse1_sgmii1g_info)
},
- { PCI_DEVICE_DATA(INTEL, TGL_SGMII1G_ID, &tgl_sgmii1g_phy0_info) },
- { PCI_DEVICE_DATA(INTEL, TGLH_SGMII1G_0_ID, &tgl_sgmii1g_phy0_info) },
- { PCI_DEVICE_DATA(INTEL, TGLH_SGMII1G_1_ID, &tgl_sgmii1g_phy1_info) },
- { PCI_DEVICE_DATA(INTEL, ADLS_SGMII1G_0_ID, &adls_sgmii1g_phy0_info) },
- { PCI_DEVICE_DATA(INTEL, ADLS_SGMII1G_1_ID, &adls_sgmii1g_phy1_info) },
+ { PCI_DEVICE_DATA(INTEL, QUARK, &quark_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_RGMII1G, &ehl_rgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_SGMII1G, &ehl_sgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_SGMII2G5, &ehl_sgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_PSE0_RGMII1G, &ehl_pse0_rgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_PSE0_SGMII1G, &ehl_pse0_sgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_PSE0_SGMII2G5, &ehl_pse0_sgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_PSE1_RGMII1G, &ehl_pse1_rgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_PSE1_SGMII1G, &ehl_pse1_sgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, EHL_PSE1_SGMII2G5, &ehl_pse1_sgmii1g_info) },
+ { PCI_DEVICE_DATA(INTEL, TGL_SGMII1G, &tgl_sgmii1g_phy0_info) },
+ { PCI_DEVICE_DATA(INTEL, TGLH_SGMII1G_0, &tgl_sgmii1g_phy0_info) },
+ { PCI_DEVICE_DATA(INTEL, TGLH_SGMII1G_1, &tgl_sgmii1g_phy1_info)
Re: [PATCH net-next v2 0/5] net: pch: fix and a few cleanups
On Thu, Mar 25, 2021 at 07:34:07PM +0200, Andy Shevchenko wrote: > The series provides one fix (patch 1) for GPIO to be able to wait for > the GPIO driver to appear. This is separated from the conversion to > the GPIO descriptors (patch 2) in order to have a possibility for > backporting. Patches 3 and 4 fix a minor warnings from Sparse while > moving to a new APIs. Patch 5 is MODULE_VERSION() clean up. > > Tested on Intel Minnowboard (v1). Anything should I do here? -- With Best Regards, Andy Shevchenko
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Fri, Mar 26, 2021 at 04:26:55PM -0500, Bjorn Helgaas wrote: > [+cc Randy, Andrew (though I'm sure you have zero interest in this > ancient question :))] > > On Wed, Dec 09, 2020 at 09:31:21AM +0100, Heiner Kallweit wrote: > > pci_set_mwi() and pci_try_set_mwi() do exactly the same, just that the > > former one is declared as __must_check. However also some callers of > > pci_set_mwi() have a comment that it's an optional feature. I don't > > think there's much sense in this separation and the use of > > __must_check. Therefore remove pci_try_set_mwi() and remove the > > __must_check attribute from pci_set_mwi(). > > I don't expect either function to be used in new code anyway. > > There's not much I like better than removing things. But some > significant thought went into adding pci_try_set_mwi() in the first > place, so I need a little more convincing about why it's safe to > remove it. > > The argument should cite the discussion about adding it. I think one > of the earliest conversations is here: > https://lore.kernel.org/linux-ide/20070404213704.224128ec.randy.dun...@oracle.com/ It's solely PCI feature which is absent on PCIe. So, if there is a guarantee that the driver never services a device connected to old PCI bus, it's okay to remove the call (it's no-op on PCIe anyway). OTOH, PCI core may try MWI itself for every device (but this is an opposite, what should we do on broken devices that do change their state based on that bit while violating specification). In any case Acked-by: Andy Shevchenko for DesignWare DMA case. I have added that and I never saw that IP connected to the old PCI. -- With Best Regards, Andy Shevchenko
[PATCH v2 4/5] net: pch_gbe: Use proper accessors to BE data in pch_ptp_match()
Sparse is not happy about handling of strict types in pch_ptp_match():
.../pch_gbe_main.c:158:33: warning: incorrect type in argument 2 (different
base types)
.../pch_gbe_main.c:158:33:expected unsigned short [usertype] uid_hi
.../pch_gbe_main.c:158:33:got restricted __be16 [usertype]
.../pch_gbe_main.c:158:45: warning: incorrect type in argument 3 (different
base types)
.../pch_gbe_main.c:158:45:expected unsigned int [usertype] uid_lo
.../pch_gbe_main.c:158:45:got restricted __be32 [usertype]
.../pch_gbe_main.c:158:56: warning: incorrect type in argument 4 (different
base types)
.../pch_gbe_main.c:158:56:expected unsigned short [usertype] seqid
.../pch_gbe_main.c:158:56:got restricted __be16 [usertype]
Fix that by switching to use proper accessors to BE data.
Signed-off-by: Andy Shevchenko
---
.../ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 91de7faa6dfe..b54e73c57d65 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -108,7 +108,7 @@ static int pch_ptp_match(struct sk_buff *skb, u16 uid_hi,
u32 uid_lo, u16 seqid)
{
u8 *data = skb->data;
unsigned int offset;
- u16 *hi, *id;
+ u16 hi, id;
u32 lo;
if (ptp_classify_raw(skb) == PTP_CLASS_NONE)
@@ -119,14 +119,11 @@ static int pch_ptp_match(struct sk_buff *skb, u16 uid_hi,
u32 uid_lo, u16 seqid)
if (skb->len < offset + OFF_PTP_SEQUENCE_ID + sizeof(seqid))
return 0;
- hi = (u16 *)(data + offset + OFF_PTP_SOURCE_UUID);
- id = (u16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
+ hi = get_unaligned_be16(data + offset + OFF_PTP_SOURCE_UUID + 0);
+ lo = get_unaligned_be32(data + offset + OFF_PTP_SOURCE_UUID + 2);
+ id = get_unaligned_be16(data + offset + OFF_PTP_SEQUENCE_ID);
- memcpy(&lo, &hi[1], sizeof(lo));
-
- return (uid_hi == *hi &&
- uid_lo == lo &&
- seqid == *id);
+ return (uid_hi == hi && uid_lo == lo && seqid == id);
}
static void
@@ -136,7 +133,6 @@ pch_rx_timestamp(struct pch_gbe_adapter *adapter, struct
sk_buff *skb)
struct pci_dev *pdev;
u64 ns;
u32 hi, lo, val;
- u16 uid, seq;
if (!adapter->hwts_rx_en)
return;
@@ -152,10 +148,7 @@ pch_rx_timestamp(struct pch_gbe_adapter *adapter, struct
sk_buff *skb)
lo = pch_src_uuid_lo_read(pdev);
hi = pch_src_uuid_hi_read(pdev);
- uid = hi & 0x;
- seq = (hi >> 16) & 0x;
-
- if (!pch_ptp_match(skb, htons(uid), htonl(lo), htons(seq)))
+ if (!pch_ptp_match(skb, hi, lo, hi >> 16))
goto out;
ns = pch_rx_snap_read(pdev);
--
2.30.2
[PATCH v2 3/5] net: pch_gbe: use readx_poll_timeout_atomic() variant
Use readx_poll_timeout_atomic() instead of open coded variants.
While at it, add __iomem attribute to the parameter of pch_gbe_wait_clr_bit().
Signed-off-by: Andy Shevchenko
---
.../ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 27 ++-
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 1038947cbac8..91de7faa6dfe 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -11,6 +11,7 @@
#include
#include
+#include
#include
#include
#include
@@ -297,15 +298,12 @@ static s32 pch_gbe_mac_read_mac_addr(struct pch_gbe_hw
*hw)
* @reg: Pointer of register
* @bit: Busy bit
*/
-static void pch_gbe_wait_clr_bit(void *reg, u32 bit)
+static void pch_gbe_wait_clr_bit(void __iomem *reg, u32 bit)
{
u32 tmp;
/* wait busy */
- tmp = 1000;
- while ((ioread32(reg) & bit) && --tmp)
- cpu_relax();
- if (!tmp)
+ if (readx_poll_timeout_atomic(ioread32, reg, tmp, !(tmp & bit), 0, 10))
pr_err("Error: busy bit is not cleared\n");
}
@@ -489,18 +487,13 @@ u16 pch_gbe_mac_ctrl_miim(struct pch_gbe_hw *hw, u32
addr, u32 dir, u32 reg,
u16 data)
{
struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
- u32 data_out = 0;
- unsigned int i;
unsigned long flags;
+ u32 data_out;
spin_lock_irqsave(&hw->miim_lock, flags);
- for (i = 100; i; --i) {
- if ((ioread32(&hw->reg->MIIM) & PCH_GBE_MIIM_OPER_READY))
- break;
- udelay(20);
- }
- if (i == 0) {
+ if (readx_poll_timeout_atomic(ioread32, &hw->reg->MIIM, data_out,
+ data_out & PCH_GBE_MIIM_OPER_READY, 20,
2000)) {
netdev_err(adapter->netdev, "pch-gbe.miim won't go Ready\n");
spin_unlock_irqrestore(&hw->miim_lock, flags);
return 0; /* No way to indicate timeout error */
@@ -508,12 +501,8 @@ u16 pch_gbe_mac_ctrl_miim(struct pch_gbe_hw *hw, u32 addr,
u32 dir, u32 reg,
iowrite32(((reg << PCH_GBE_MIIM_REG_ADDR_SHIFT) |
(addr << PCH_GBE_MIIM_PHY_ADDR_SHIFT) |
dir | data), &hw->reg->MIIM);
- for (i = 0; i < 100; i++) {
- udelay(20);
- data_out = ioread32(&hw->reg->MIIM);
- if ((data_out & PCH_GBE_MIIM_OPER_READY))
- break;
- }
+ readx_poll_timeout_atomic(ioread32, &hw->reg->MIIM, data_out,
+ data_out & PCH_GBE_MIIM_OPER_READY, 20, 2000);
spin_unlock_irqrestore(&hw->miim_lock, flags);
netdev_dbg(adapter->netdev, "PHY %s: reg=%d, data=0x%04X\n",
--
2.30.2
[PATCH v2 5/5] net: pch_gbe: remove unneeded MODULE_VERSION() call
Remove MODULE_VERSION(), as it doesn't seem to serve any practical purpose.
For in-tree drivers, the kernel version matters. The code received lots of
changes, but module version remained constant, since the driver landed in
mainline. So, this version doesn't seem have any practical meaning anymore.
Signed-off-by: Andy Shevchenko
---
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 2 --
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c | 2 ++
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c| 4
3 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index 55cef5b16aa5..0a89117a8fb4 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -596,8 +596,6 @@ struct pch_gbe_adapter {
#define pch_gbe_hw_to_adapter(hw) container_of(hw, struct
pch_gbe_adapter, hw)
-extern const char pch_driver_version[];
-
/* pch_gbe_main.c */
int pch_gbe_up(struct pch_gbe_adapter *adapter);
void pch_gbe_down(struct pch_gbe_adapter *adapter);
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
index a58f14aca10c..660b07cb5b92 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
@@ -8,6 +8,8 @@
#include "pch_gbe.h"
#include "pch_gbe_phy.h"
+static const char pch_driver_version[] = "1.01";
+
/*
* pch_gbe_stats - Stats item information
*/
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index b54e73c57d65..0c5c4fefc8af 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -16,9 +16,6 @@
#include
#include
-#define DRV_VERSION "1.01"
-const char pch_driver_version[] = DRV_VERSION;
-
#define PCH_GBE_MAR_ENTRIES16
#define PCH_GBE_SHORT_PKT 64
#define DSC_INIT16 0xC000
@@ -2726,7 +2723,6 @@ module_pci_driver(pch_gbe_driver);
MODULE_DESCRIPTION("EG20T PCH Gigabit ethernet Driver");
MODULE_AUTHOR("LAPIS SEMICONDUCTOR, ");
MODULE_LICENSE("GPL");
-MODULE_VERSION(DRV_VERSION);
MODULE_DEVICE_TABLE(pci, pch_gbe_pcidev_id);
/* pch_gbe_main.c */
--
2.30.2
[PATCH v2 2/5] net: pch_gbe: Convert to use GPIO descriptors
This switches the PCH GBE driver to use GPIO descriptors.
Signed-off-by: Andy Shevchenko
---
.../ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 45 +--
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 1b32a43f7024..1038947cbac8 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -8,10 +8,12 @@
#include "pch_gbe.h"
#include "pch_gbe_phy.h"
+
+#include
+#include
#include
#include
#include
-#include
#define DRV_VERSION "1.01"
const char pch_driver_version[] = DRV_VERSION;
@@ -96,8 +98,6 @@ const char pch_driver_version[] = DRV_VERSION;
#define PTP_L4_MULTICAST_SA "01:00:5e:00:01:81"
#define PTP_L2_MULTICAST_SA "01:1b:19:00:00:00"
-#define MINNOW_PHY_RESET_GPIO 13
-
static int pch_gbe_mdio_read(struct net_device *netdev, int addr, int reg);
static void pch_gbe_mdio_write(struct net_device *netdev, int addr, int reg,
int data);
@@ -2627,26 +2627,45 @@ static int pch_gbe_probe(struct pci_dev *pdev,
return ret;
}
+static void pch_gbe_gpio_remove_table(void *table)
+{
+ gpiod_remove_lookup_table(table);
+}
+
+static int pch_gbe_gpio_add_table(struct device *dev, void *table)
+{
+ gpiod_add_lookup_table(table);
+ return devm_add_action_or_reset(dev, pch_gbe_gpio_remove_table, table);
+}
+
+static struct gpiod_lookup_table pch_gbe_minnow_gpio_table = {
+ .dev_id = ":02:00.1",
+ .table = {
+ GPIO_LOOKUP("sch_gpio.33158", 13, NULL, GPIO_ACTIVE_LOW),
+ {}
+ },
+};
+
/* The AR803X PHY on the MinnowBoard requires a physical pin to be toggled to
* ensure it is awake for probe and init. Request the line and reset the PHY.
*/
static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
{
- unsigned long flags = GPIOF_OUT_INIT_HIGH;
- unsigned gpio = MINNOW_PHY_RESET_GPIO;
+ struct gpio_desc *gpiod;
int ret;
- ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
- "minnow_phy_reset");
- if (ret) {
- dev_err(&pdev->dev,
- "ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
+ ret = pch_gbe_gpio_add_table(&pdev->dev, &pch_gbe_minnow_gpio_table);
+ if (ret)
return ret;
- }
- gpio_set_value(gpio, 0);
+ gpiod = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_HIGH);
+ if (IS_ERR(gpiod))
+ return dev_err_probe(&pdev->dev, PTR_ERR(gpiod),
+"Can't request PHY reset GPIO line\n");
+
+ gpiod_set_value(gpiod, 1);
usleep_range(1250, 1500);
- gpio_set_value(gpio, 1);
+ gpiod_set_value(gpiod, 0);
usleep_range(1250, 1500);
return ret;
--
2.30.2
[PATCH net-next v2 0/5] net: pch: fix and a few cleanups
The series provides one fix (patch 1) for GPIO to be able to wait for the GPIO driver to appear. This is separated from the conversion to the GPIO descriptors (patch 2) in order to have a possibility for backporting. Patches 3 and 4 fix a minor warnings from Sparse while moving to a new APIs. Patch 5 is MODULE_VERSION() clean up. Tested on Intel Minnowboard (v1). Since v2: - added a few cleanups on top of the fix Andy Shevchenko (5): net: pch_gbe: Propagate error from devm_gpio_request_one() net: pch_gbe: Convert to use GPIO descriptors net: pch_gbe: use readx_poll_timeout_atomic() variant net: pch_gbe: Use proper accessors to BE data in pch_ptp_match() net: pch_gbe: remove unneeded MODULE_VERSION() call .../net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 2 - .../oki-semi/pch_gbe/pch_gbe_ethtool.c| 2 + .../ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 103 +- 3 files changed, 54 insertions(+), 53 deletions(-) -- 2.30.2
[PATCH v2 1/5] net: pch_gbe: Propagate error from devm_gpio_request_one()
If GPIO controller is not available yet we need to defer
the probe of GBE until provider will become available.
While here, drop GPIOF_EXPORT because it's deprecated and
may not be available.
Fixes: f1a26fdf5944 ("pch_gbe: Add MinnowBoard support")
Signed-off-by: Andy Shevchenko
---
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 140cee7c459d..1b32a43f7024 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -2531,9 +2531,13 @@ static int pch_gbe_probe(struct pci_dev *pdev,
adapter->pdev = pdev;
adapter->hw.back = adapter;
adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
+
adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
- if (adapter->pdata && adapter->pdata->platform_init)
- adapter->pdata->platform_init(pdev);
+ if (adapter->pdata && adapter->pdata->platform_init) {
+ ret = adapter->pdata->platform_init(pdev);
+ if (ret)
+ goto err_free_netdev;
+ }
adapter->ptp_pdev =
pci_get_domain_bus_and_slot(pci_domain_nr(adapter->pdev->bus),
@@ -2628,7 +2632,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
*/
static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
{
- unsigned long flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
+ unsigned long flags = GPIOF_OUT_INIT_HIGH;
unsigned gpio = MINNOW_PHY_RESET_GPIO;
int ret;
--
2.30.2
Re: [PATCH net v1 1/1] net: pch_gbe: Propagate error from devm_gpio_request_one()
On Wed, Mar 24, 2021 at 11:23:10PM +0200, Andy Shevchenko wrote: > If GPIO controller is not available yet we need to defer > the probe of GBE until provider will become available. > > While here, drop GPIOF_EXPORT because it's deprecated and > may not be available. I'll send a v2 with many patches against this driver, so I will include this fix there as well. -- With Best Regards, Andy Shevchenko
[PATCH net v1 1/1] net: pch_gbe: Propagate error from devm_gpio_request_one()
If GPIO controller is not available yet we need to defer
the probe of GBE until provider will become available.
While here, drop GPIOF_EXPORT because it's deprecated and
may not be available.
Fixes: f1a26fdf5944 ("pch_gbe: Add MinnowBoard support")
Signed-off-by: Andy Shevchenko
---
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 140cee7c459d..1b32a43f7024 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -2531,9 +2531,13 @@ static int pch_gbe_probe(struct pci_dev *pdev,
adapter->pdev = pdev;
adapter->hw.back = adapter;
adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
+
adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
- if (adapter->pdata && adapter->pdata->platform_init)
- adapter->pdata->platform_init(pdev);
+ if (adapter->pdata && adapter->pdata->platform_init) {
+ ret = adapter->pdata->platform_init(pdev);
+ if (ret)
+ goto err_free_netdev;
+ }
adapter->ptp_pdev =
pci_get_domain_bus_and_slot(pci_domain_nr(adapter->pdev->bus),
@@ -2628,7 +2632,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
*/
static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
{
- unsigned long flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
+ unsigned long flags = GPIOF_OUT_INIT_HIGH;
unsigned gpio = MINNOW_PHY_RESET_GPIO;
int ret;
--
2.30.2
Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
On Wed, Mar 17, 2021 at 11:36 AM Andy Shevchenko wrote: > On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong > wrote: ... > It maybe fixed by swapping positions of the arguments, i.e. ~(FOO | > BAR) & flags. ...and type casting will be needed anyway here... I was thinking about this case drivers/i2c/busses/i2c-designware-common.c:420: dev->sda_hold_time & ~(u32)DW_IC_SDA_HOLD_RX_MASK , but sda_hold_time there is unsigned. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong wrote: > On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck wrote: > > On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote: > > > On Wednesday, March 17, 2021, Guenter Roeck wrote: > > > > ... > > > > The problem is in net/packet/af_packet.c:packet_recvmsg(). This function, > > as well as all other rcvmsg functions, is declared as > > > > static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t > > len, > > int flags) > > > > MSG_CMSG_COMPAT (0x8000) is set in flags, meaning its value is negative. > > This is then evaluated in > > > >if (flags & > > ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE)) > > goto out; > > > > If any of those flags is declared as BIT() and thus long, flags is > > sign-extended to long. Since it is negative, its upper 32 bits will be set, > > the if statement evaluates as true, and the function bails out. > > > > This is relatively easy to fix here with, for example, > > > > if ((unsigned int)flags & > > ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE)) > > goto out; > > > > but that is just a hack, and it doesn't solve the real problem: > > Each function in struct proto_ops which passes flags passes it as int > > (see include/linux/net.h:struct proto_ops). Each such function, if > > called with MSG_CMSG_COMPAT set, will fail a match against > > ~(MSG_anything) if MSG_anything is declared as BIT() or long. > > > > As it turns out, I was kind of lucky to catch the problem: So far I have > > seen it only on mips64 kernels with N32 userspace. > > > > Guenter > > Wow, now the usages of 'msg_flag' really puzzle me. Seems that > it is used as 'unsigned int' somewhere, but 'int' somewhere > else. > > As I found, It is used as 'int' in 'netlink_recvmsg()', > 'io_sr_msg->msg_flags', 'atalk_sendmsg()', > 'dn_recvmsg()', 'proto_ops->recvmsg()', etc. > > So what should I do? Revert this patch? Or fix the usages of 'flags'? > Or change the type of MSG_* to 'unsigned int'? I prefer the last > one(the usages of 'flags' can be fixed too, maybe later). The problematic code is negation of the flags when it's done in operations like &. It maybe fixed by swapping positions of the arguments, i.e. ~(FOO | BAR) & flags. All this is a beast called "integer promotions" in the C standard. The best is to try to get flags to be unsigned. By how invasive it may be? -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v7 08/16] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
On Thu, Mar 11, 2021 at 8:00 PM Calvin Johnson wrote: > On Thu, Mar 11, 2021 at 02:09:37PM +0200, Andy Shevchenko wrote: > > On Thu, Mar 11, 2021 at 8:21 AM Calvin Johnson > > wrote: ... > > > +config FWNODE_MDIO > > > + def_tristate PHYLIB > > > > (Seems "selectable only" item) > > What do you mean by "selectable only" item here? Can you please point to some > other example? The Kconfig sections without descriptions are not user-visible. No user can run menuconfig and check a box with "I want this to be compiled". tristate // selectable-only tristate "bla bla bla" // user visible and selectable > > > + depends on ACPI > > > + depends on OF > > > > Wouldn't be better to have > > depends on (ACPI || OF) || COMPILE_TEST > > > > And honestly I don't understand it in either (AND or OR) variant. Why > > do you need a dependency like this for fwnode API? > > Here, fwnode_mdiobus_register_phy() uses objects from both ACPI and OF. APIs? Calls? What really fails if we have !ACPI and / or !OF? > > Moreover dependencies don't work for "selectable only" items. > > > > > + depends on PHYLIB > > > + select FIXED_PHY -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v7 11/16] net: mdio: Add ACPI support code for mdio
dev,
> + "MDIO device at address %d is missing.\n",
> + addr);
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(acpi_mdiobus_register);
> diff --git a/include/linux/acpi_mdio.h b/include/linux/acpi_mdio.h
> new file mode 100644
> index ..748d261fe2f9
> --- /dev/null
> +++ b/include/linux/acpi_mdio.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * ACPI helper for the MDIO (Ethernet PHY) API
> + */
> +
> +#ifndef __LINUX_ACPI_MDIO_H
> +#define __LINUX_ACPI_MDIO_H
> +
> +#include
> +
> +#if IS_ENABLED(CONFIG_ACPI_MDIO)
> +int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle
> *fwnode);
> +#else /* CONFIG_ACPI_MDIO */
> +static inline int acpi_mdiobus_register(struct mii_bus *mdio, struct
> fwnode_handle *fwnode)
> +{
> + /*
> +* Fall back to mdiobus_register() function to register a bus.
> +* This way, we don't have to keep compat bits around in drivers.
> +*/
> +
> + return mdiobus_register(mdio);
> +}
> +#endif
> +
> +#endif /* __LINUX_ACPI_MDIO_H */
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v7 10/16] ACPI: utils: Introduce acpi_get_local_address()
On Thu, Mar 11, 2021 at 8:22 AM Calvin Johnson
wrote:
>
> Introduce a wrapper around the _ADR evaluation.
Reviewed-by: Andy Shevchenko
> Signed-off-by: Calvin Johnson
> ---
>
> Changes in v7: None
> Changes in v6: None
> Changes in v5:
> - Replace fwnode_get_id() with acpi_get_local_address()
>
> Changes in v4:
> - Improve code structure to handle all cases
>
> Changes in v3:
> - Modified to retrieve reg property value for ACPI as well
> - Resolved compilation issue with CONFIG_ACPI = n
> - Added more info into documentation
>
> Changes in v2: None
>
> drivers/acpi/utils.c | 14 ++
> include/linux/acpi.h | 7 +++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 682edd913b3b..41fe380a09a7 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -277,6 +277,20 @@ acpi_evaluate_integer(acpi_handle handle,
>
> EXPORT_SYMBOL(acpi_evaluate_integer);
>
> +int acpi_get_local_address(acpi_handle handle, u32 *addr)
> +{
> + unsigned long long adr;
> + acpi_status status;
> +
> + status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
> + if (ACPI_FAILURE(status))
> + return -ENODATA;
> +
> + *addr = (u32)adr;
> + return 0;
> +}
> +EXPORT_SYMBOL(acpi_get_local_address);
> +
> acpi_status
> acpi_evaluate_reference(acpi_handle handle,
> acpi_string pathname,
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index fcdaab723916..700f9fc303ab 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -706,6 +706,8 @@ static inline u64 acpi_arch_get_root_pointer(void)
> }
> #endif
>
> +int acpi_get_local_address(acpi_handle handle, u32 *addr);
> +
> #else /* !CONFIG_ACPI */
>
> #define acpi_disabled 1
> @@ -953,6 +955,11 @@ static inline struct acpi_device
> *acpi_resource_consumer(struct resource *res)
> return NULL;
> }
>
> +static inline int acpi_get_local_address(acpi_handle handle, u32 *addr)
> +{
> + return -ENODEV;
> +}
> +
> #endif /* !CONFIG_ACPI */
>
> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v7 08/16] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
fwnode_get_phy_id(child, &phy_id))
> + phy = get_phy_device(bus, addr, is_c45);
> + else
> + phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> + if (IS_ERR(phy)) {
> + unregister_mii_timestamper(mii_ts);
> + return PTR_ERR(phy);
> + }
> +
> + if (is_acpi_node(child)) {
> + phy->irq = bus->irq[addr];
> +
> + /* Associate the fwnode with the device structure so it
> +* can be looked up later.
> +*/
> + phy->mdio.dev.fwnode = child;
> +
> + /* All data is now stored in the phy struct, so register it */
> + rc = phy_device_register(phy);
> + if (rc) {
> + phy_device_free(phy);
> + fwnode_handle_put(phy->mdio.dev.fwnode);
> + return rc;
> + }
> + } else if (is_of_node(child)) {
> + rc = of_mdiobus_phy_device_register(bus, phy,
> to_of_node(child), addr);
> + if (rc) {
> + unregister_mii_timestamper(mii_ts);
> + phy_device_free(phy);
> + return rc;
> + }
> + }
> +
> + /* phy->mii_ts may already be defined by the PHY driver. A
> +* mii_timestamper probed via the device tree will still have
> +* precedence.
> +*/
> + if (mii_ts)
> + phy->mii_ts = mii_ts;
> + return 0;
> +}
> +EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> index 48b6b8458c17..db293e0b8249 100644
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
> @@ -32,7 +32,7 @@ static int of_get_phy_id(struct device_node *device, u32
> *phy_id)
> return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
> }
>
> -static struct mii_timestamper *of_find_mii_timestamper(struct device_node
> *node)
> +struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
> {
> struct of_phandle_args arg;
> int err;
> @@ -49,6 +49,7 @@ static struct mii_timestamper
> *of_find_mii_timestamper(struct device_node *node)
>
> return register_mii_timestamper(arg.np, arg.args[0]);
> }
> +EXPORT_SYMBOL(of_find_mii_timestamper);
>
> int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device
> *phy,
> struct device_node *child, u32 addr)
> diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h
> new file mode 100644
> index ..8c0392845916
> --- /dev/null
> +++ b/include/linux/fwnode_mdio.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * FWNODE helper for the MDIO (Ethernet PHY) API
> + */
> +
> +#ifndef __LINUX_FWNODE_MDIO_H
> +#define __LINUX_FWNODE_MDIO_H
> +
> +#include
> +
> +#if IS_ENABLED(CONFIG_FWNODE_MDIO)
> +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> + struct fwnode_handle *child, u32 addr);
> +
> +#else /* CONFIG_FWNODE_MDIO */
> +static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> + struct fwnode_handle *child,
> + u32 addr)
> +{
> + return -EINVAL;
> +}
> +#endif
> +
> +#endif /* __LINUX_FWNODE_MDIO_H */
> diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
> index 2b05e7f7c238..e4ee6c4d9431 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -31,6 +31,7 @@ struct mii_bus *of_mdio_find_bus(struct device_node
> *mdio_np);
> int of_phy_register_fixed_link(struct device_node *np);
> void of_phy_deregister_fixed_link(struct device_node *np);
> bool of_phy_is_fixed_link(struct device_node *np);
> +struct mii_timestamper *of_find_mii_timestamper(struct device_node *np);
> int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device
> *phy,
>struct device_node *child, u32 addr);
>
> @@ -118,7 +119,10 @@ static inline bool of_phy_is_fixed_link(struct
> device_node *np)
> {
> return false;
> }
> -
> +static inline struct mii_timestamper *of_find_mii_timestamper(struct
> device_node *np)
> +{
> + return NULL;
> +}
> static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
> struct phy_device *phy,
> struct device_node *child, u32
> addr)
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v7 07/16] net: mii_timestamper: check NULL in unregister_mii_timestamper()
On Thu, Mar 11, 2021 at 8:21 AM Calvin Johnson
wrote:
>
> Callers of unregister_mii_timestamper() currently check for NULL
> value of mii_ts before calling it.
>
> Place the NULL check inside unregister_mii_timestamper() and update
> the callers accordingly
FWIW,
Reviewed-by: Andy Shevchenko
(Don't remember if it has been suggested by somebody, in that case
perhaps Suggested-by?)
> Signed-off-by: Calvin Johnson
> ---
>
> Changes in v7:
> - check NULL in unregister_mii_timestamper()
>
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/net/mdio/of_mdio.c| 6 ++
> drivers/net/phy/mii_timestamper.c | 3 +++
> drivers/net/phy/phy_device.c | 3 +--
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> index 612a37970f14..48b6b8458c17 100644
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
> @@ -115,15 +115,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
> else
> phy = get_phy_device(mdio, addr, is_c45);
> if (IS_ERR(phy)) {
> - if (mii_ts)
> - unregister_mii_timestamper(mii_ts);
> + unregister_mii_timestamper(mii_ts);
> return PTR_ERR(phy);
> }
>
> rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
> if (rc) {
> - if (mii_ts)
> - unregister_mii_timestamper(mii_ts);
> + unregister_mii_timestamper(mii_ts);
> phy_device_free(phy);
> return rc;
> }
> diff --git a/drivers/net/phy/mii_timestamper.c
> b/drivers/net/phy/mii_timestamper.c
> index b71b7456462d..51ae0593a04f 100644
> --- a/drivers/net/phy/mii_timestamper.c
> +++ b/drivers/net/phy/mii_timestamper.c
> @@ -111,6 +111,9 @@ void unregister_mii_timestamper(struct mii_timestamper
> *mii_ts)
> struct mii_timestamping_desc *desc;
> struct list_head *this;
>
> + if (!mii_ts)
> + return;
> +
> /* mii_timestamper statically registered by the PHY driver won't use
> the
> * register_mii_timestamper() and thus don't have ->device set. Don't
> * try to unregister these.
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index f875efe7b4d1..9c5127405d91 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -928,8 +928,7 @@ EXPORT_SYMBOL(phy_device_register);
> */
> void phy_device_remove(struct phy_device *phydev)
> {
> - if (phydev->mii_ts)
> - unregister_mii_timestamper(phydev->mii_ts);
> + unregister_mii_timestamper(phydev->mii_ts);
>
> device_del(&phydev->mdio.dev);
>
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 net-next] net: socket: use BIT() for MSG_*
On Tue, Mar 9, 2021 at 5:29 PM Menglong Dong wrote: > On Wed, Feb 24, 2021 at 4:30 AM Jakub Kicinski wrote: > > > ... > > Please repost when net-next reopens after 5.12-rc1 is cut. > > > > Look out for the announcement on the mailing list or check: > > http://vger.kernel.org/~davem/net-next.html > > > > RFC patches sent for review only are obviously welcome at any time. > > Is 'net-next' open? Can I resend this patch now? It seems that a long > time has passed. Should be. Usually the merge window takes from the last release (v5.11) till the first rc of the new cycle (v5.12-rc1). Now we are more than a week after v5.12-rc1. -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v6 10/15] net: mdio: Add ACPI support code for mdio
On Mon, Mar 8, 2021 at 6:28 PM Calvin Johnson wrote: > On Mon, Mar 08, 2021 at 04:57:35PM +0200, Andy Shevchenko wrote: > I thought of including device.h instead of dev_printk.h because, it is the > only file that includes dev_printk.h and device.h is widely used. Of course, > it will mean that dev_printk.h is indirectly called. The split happened recently, not every developer knows about it and definitely most of the contributors are too lazy to properly write the inclusion block in their code. -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v6 10/15] net: mdio: Add ACPI support code for mdio
On Mon, Mar 8, 2021 at 4:11 PM Calvin Johnson wrote: > On Thu, Feb 18, 2021 at 05:08:05PM +0200, Andy Shevchenko wrote: > > On Thu, Feb 18, 2021 at 7:28 AM Calvin Johnson > > wrote: > > > Define acpi_mdiobus_register() to Register mii_bus and create PHYs for > > > each ACPI child node. > > > > > +#include > > > +#include > > > > Perhaps it's better to provide the headers that this file is direct > > user of, i.e. > > bits.h > > dev_printk.h > > Looks like device.h needs to be used instead of dev_printk.h. Please > let me know if you've a different opinion. I don't see the user of device.h. dev_printk.h is definitely in use here... Do you see a user for device.h? Which line in your code requires it? It might be that I don't see something quite obvious... > > module.h > > types.h > > > > The rest seems fine because they are guaranteed to be included by > > acpi.h (IIUC about fwnode API and acpi_mdio includes MDIO PHY APIs). -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v6 10/15] net: mdio: Add ACPI support code for mdio
On Thu, Feb 18, 2021 at 7:28 AM Calvin Johnson wrote: > > Define acpi_mdiobus_register() to Register mii_bus and create PHYs for > each ACPI child node. > +#include > +#include Perhaps it's better to provide the headers that this file is direct user of, i.e. bits.h dev_printk.h module.h types.h The rest seems fine because they are guaranteed to be included by acpi.h (IIUC about fwnode API and acpi_mdio includes MDIO PHY APIs). -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v6 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Thu, Feb 18, 2021 at 7:29 AM Calvin Johnson wrote: > > Modify dpaa2_mac_get_node() to get the dpmac fwnode from either > DT or ACPI. > > Modify dpaa2_mac_get_if_mode() to get interface mode from dpmac_node > which is a fwnode. > > Modify dpaa2_pcs_create() to create pcs from dpmac_node fwnode. > > Modify dpaa2_mac_connect() to support ACPI along with DT. ... > + if (is_of_node(fwnode)) Redundant check I think. If it's not an fwnode, the dpmacs is NULL and of_node_put() is NULL-aware. > + of_node_put(dpmacs); ... > + if (is_of_node(fwnode)) > + of_node_put(dpmacs); Ditto. ... > mac->if_link_type = mac->attr.link_type; > - Do we need to remove this blank line? ... > + if (is_of_node(dpmac_node)) > + fwnode_handle_put(dpmac_node); > + if (is_of_node(dpmac_node)) > + fwnode_handle_put(dpmac_node); Also not sure that you need a check in the above code excerpts. -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v6 07/15] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
On Thu, Feb 18, 2021 at 7:28 AM Calvin Johnson wrote: > > Introduce fwnode_mdiobus_register_phy() to register PHYs on the > mdiobus. From the compatible string, identify whether the PHY is > c45 and based on this create a PHY device instance which is > registered on the mdiobus. Thanks for an update! Below some nit-picks, may be ignored. > uninitialized symbol 'mii_ts' > Reported-by: kernel test robot > Reported-by: Dan Carpenter I don't think the above deserves to be in the commit message (rather after the cutter '---' line as a changelog). > Signed-off-by: Calvin Johnson ... > + if (mii_ts) > + unregister_mii_timestamper(mii_ts); ... > + if (mii_ts) > + unregister_mii_timestamper(mii_ts); I'm wondering if we can move this check to the unregister_mii_timestamper() to make it NULL aware as many other similar (unregister) APIs do. I have checked that there are currently three users of this that can benefit of the change. ... > + /* phy->mii_ts may already be defined by the PHY driver. A > +* mii_timestamper probed via the device tree will still have > +* precedence. > +*/ > + if (mii_ts) > + phy->mii_ts = mii_ts; I'm wondering if the belo form is better to read phy->mii_ts = mii_ts ?: phy->mii_ts; -- With Best Regards, Andy Shevchenko
Re: [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
On Wed, Feb 17, 2021 at 01:15:43PM +0100, Enrico Weigelt, metux IT consult
wrote:
> Under rare circumstances it may happen that a device node's name is NULL
> (most likely kernel bug in some other place).
What circumstances? How can I reproduce this? More information, please!
> In such situations anything
> but helpful, if the debug printout crashes, and nobody knows what actually
> happened here.
>
> Therefore protect it by an explicit NULL check and print out an extra
> warning.
...
> + pr_warn("device_node without name. Kernel bug
> ?\n");
If it's not once, then it's possible to have log spammed with this, right?
...
> + p = "";
We have different standard de facto for NULL pointers to be printed. Actually
if you wish, you may gather them under one definition (maybe somewhere under
printk) and export to everybody to use.
--
With Best Regards,
Andy Shevchenko
Re: commit 0f0aefd733f7 to linux-firmware effectively broke all of the setups with old kernels
On Mon, Feb 15, 2021 at 8:03 PM Peter Robinson wrote: > > > Seems the commit 0f0aefd733f7 to linux-firmware effectively broke all > > of the setups with the old kernels. Firmware name is an ABI (!) and > > replacing it like this will definitely break systems with older > > kernels. Linux firmware package likely, but unfortunately, should > > carry on both versions as long as it's needed. Alternative solution is > > to provide the links during installation. > > It does provide the links using the copy-firmware.sh and the details in > WHENCE. > > The alternative is to leave firmwares in place with CVEs. Good, thanks, I haven't looked into that script. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/3] string: Consolidate yesno() helpers under string.h hood
On Mon, Feb 15, 2021 at 04:37:50PM +0200, Jani Nikula wrote: > On Mon, 15 Feb 2021, Andy Shevchenko > wrote: > > We have already few similar implementation and a lot of code that can > > benefit > > of the yesno() helper. Consolidate yesno() helpers under string.h hood. > > Good luck. I gave up after just four versions. [1] Thanks for a pointer! I like your version, but here we also discussing a possibility to do something like %py[DOY]. It will consolidate all those RO or whatever sections inside one data structure. > Acked-by: Jani Nikula > > [1] http://lore.kernel.org/r/20191023131308.9420-1-jani.nik...@intel.com -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 15, 2021 at 5:13 PM Andy Shevchenko
wrote:
>
> On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson
> wrote:
> > On Mon, Feb 08, 2021 at 04:28:31PM +, Russell King - ARM Linux admin
> > wrote:
>
> ...
>
> > I think of_phy_is_fixed_link() needs to be fixed. I'll add below fix.
> >
> > --- a/drivers/net/mdio/of_mdio.c
> > +++ b/drivers/net/mdio/of_mdio.c
> > @@ -439,6 +439,9 @@ bool of_phy_is_fixed_link(struct device_node *np)
> > int len, err;
> > const char *managed;
> >
> > + if (!np)
> > + return false;
>
> AFAICS this doesn't add anything: all of the of_* APIs should handle
> OF nodes being NULL below.
>
> > /* New binding */
> > dn = of_get_child_by_name(np, "fixed-link");
> > if (dn) {
Yes, of_get_next_child() and of_get_property() are NULL aware.
So, the check is redundant.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson
wrote:
> On Mon, Feb 08, 2021 at 04:28:31PM +, Russell King - ARM Linux admin
> wrote:
...
> I think of_phy_is_fixed_link() needs to be fixed. I'll add below fix.
>
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
> @@ -439,6 +439,9 @@ bool of_phy_is_fixed_link(struct device_node *np)
> int len, err;
> const char *managed;
>
> + if (!np)
> + return false;
AFAICS this doesn't add anything: all of the of_* APIs should handle
OF nodes being NULL below.
> /* New binding */
> dn = of_get_child_by_name(np, "fixed-link");
> if (dn) {
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/3] string: Consolidate yesno() helpers under string.h hood
+Cc: Sakari and printk people On Mon, Feb 15, 2021 at 4:28 PM Christian König wrote: > Am 15.02.21 um 15:21 schrieb Andy Shevchenko: > > We have already few similar implementation and a lot of code that can > > benefit > > of the yesno() helper. Consolidate yesno() helpers under string.h hood. > > > > Signed-off-by: Andy Shevchenko > > Looks like a good idea to me, feel free to add an Acked-by: Christian > König to the series. Thanks. > But looking at the use cases for this, wouldn't it make more sense to > teach kprintf some new format modifier for this? As a next step? IIRC Sakari has at some point the series converted yesno and Co. to something which I don't remember the details of. Guys, what do you think? -- With Best Regards, Andy Shevchenko
commit 0f0aefd733f7 to linux-firmware effectively broke all of the setups with old kernels
Hi! Seems the commit 0f0aefd733f7 to linux-firmware effectively broke all of the setups with the old kernels. Firmware name is an ABI (!) and replacing it like this will definitely break systems with older kernels. Linux firmware package likely, but unfortunately, should carry on both versions as long as it's needed. Alternative solution is to provide the links during installation. Btw, I haven't seen the driver change for that. Care to provide a commit ID in upstream? -- With Best Regards, Andy Shevchenko
[PATCH v1 2/3] string: Move onoff() helper under string.h hood
We have already an implementation and a lot of code that can benefit
of the onoff() helper. Move it under string.h hood.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/i915_utils.h | 5 -
include/linux/string.h| 5 +
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index e6da5a951132..d2ac357896d4 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -409,11 +409,6 @@ wait_remaining_ms_from_jiffies(unsigned long
timestamp_jiffies, int to_wait_ms)
#define MBps(x) KBps(1000 * (x))
#define GBps(x) ((u64)1000 * MBps((x)))
-static inline const char *onoff(bool v)
-{
- return v ? "on" : "off";
-}
-
static inline const char *enableddisabled(bool v)
{
return v ? "enabled" : "disabled";
diff --git a/include/linux/string.h b/include/linux/string.h
index fd946a5e18c8..2a0589e945d9 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -313,4 +313,9 @@ static inline const char *yesno(bool yes)
return yes ? "yes" : "no";
}
+static inline const char *onoff(bool on)
+{
+ return on ? "on" : "off";
+}
+
#endif /* _LINUX_STRING_H_ */
--
2.30.0
[PATCH v1 1/3] string: Consolidate yesno() helpers under string.h hood
We have already few similar implementation and a lot of code that can benefit
of the yesno() helper. Consolidate yesno() helpers under string.h hood.
Signed-off-by: Andy Shevchenko
---
.../drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c| 6 +-
drivers/gpu/drm/i915/i915_utils.h| 6 +-
drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 12 +---
include/linux/string.h | 5 +
4 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index 360952129b6d..7fde4f90e513 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -23,6 +23,7 @@
*
*/
+#include
#include
#include
@@ -49,11 +50,6 @@ struct dmub_debugfs_trace_entry {
uint32_t param1;
};
-static inline const char *yesno(bool v)
-{
- return v ? "yes" : "no";
-}
-
/* parse_write_buffer_into_params - Helper function to parse debugfs write
buffer into an array
*
* Function takes in attributes passed to debugfs write entry
diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index abd4dcd9f79c..e6da5a951132 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -27,6 +27,7 @@
#include
#include
+#include
#include
#include
#include
@@ -408,11 +409,6 @@ wait_remaining_ms_from_jiffies(unsigned long
timestamp_jiffies, int to_wait_ms)
#define MBps(x) KBps(1000 * (x))
#define GBps(x) ((u64)1000 * MBps((x)))
-static inline const char *yesno(bool v)
-{
- return v ? "yes" : "no";
-}
-
static inline const char *onoff(bool v)
{
return v ? "on" : "off";
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index 7d49fd4edc9e..c857d73abbd7 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -34,6 +34,7 @@
#include
#include
+#include
#include
#include
#include
@@ -2015,17 +2016,6 @@ static const struct file_operations rss_debugfs_fops = {
/* RSS Configuration.
*/
-/* Small utility function to return the strings "yes" or "no" if the supplied
- * argument is non-zero.
- */
-static const char *yesno(int x)
-{
- static const char *yes = "yes";
- static const char *no = "no";
-
- return x ? yes : no;
-}
-
static int rss_config_show(struct seq_file *seq, void *v)
{
struct adapter *adapter = seq->private;
diff --git a/include/linux/string.h b/include/linux/string.h
index 9521d8cab18e..fd946a5e18c8 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -308,4 +308,9 @@ static __always_inline size_t str_has_prefix(const char
*str, const char *prefix
return strncmp(str, prefix, len) == 0 ? len : 0;
}
+static inline const char *yesno(bool yes)
+{
+ return yes ? "yes" : "no";
+}
+
#endif /* _LINUX_STRING_H_ */
--
2.30.0
[PATCH v1 3/3] string: Move enableddisabled() helper under string.h hood
We have already an implementation and a lot of code that can benefit
of the enableddisabled() helper. Move it under string.h hood.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/i915/i915_utils.h | 5 -
include/linux/string.h| 5 +
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index d2ac357896d4..b05d72b4dd93 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -409,11 +409,6 @@ wait_remaining_ms_from_jiffies(unsigned long
timestamp_jiffies, int to_wait_ms)
#define MBps(x) KBps(1000 * (x))
#define GBps(x) ((u64)1000 * MBps((x)))
-static inline const char *enableddisabled(bool v)
-{
- return v ? "enabled" : "disabled";
-}
-
void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint);
static inline void __add_taint_for_CI(unsigned int taint)
{
diff --git a/include/linux/string.h b/include/linux/string.h
index 2a0589e945d9..25f055aa4c31 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -318,4 +318,9 @@ static inline const char *onoff(bool on)
return on ? "on" : "off";
}
+static inline const char *enableddisabled(bool enabled)
+{
+ return enabled ? "enabled" : "disabled";
+}
+
#endif /* _LINUX_STRING_H_ */
--
2.30.0
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 8, 2021 at 5:15 PM Calvin Johnson
wrote:
>
> Modify dpaa2_mac_connect() to support ACPI along with DT.
> Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
> DT or ACPI.
>
> Replace of_get_phy_mode with fwnode_get_phy_mode to get
> phy-mode for a dpmac_node.
>
> Use helper function phylink_fwnode_phy_connect() to find phy_dev and
> connect to mac->phylink.
...
> + if (is_of_node(dev->parent->fwnode)) {
> + dpmacs = of_find_node_by_name(NULL, "dpmacs");
> + if (!dpmacs)
> + return NULL;
> + parent = of_fwnode_handle(dpmacs);
> + } else if (is_acpi_node(dev->parent->fwnode)) {
> + parent = dev->parent->fwnode;
dev_fwnode(dev->parent) ?
> + }
...
> + if (err) {
> continue;
> + } else if (id == dpmac_id) {
Useless 'else'
> + if (is_of_node(dev->parent->fwnode))
dev_fwnode() ?
> + of_node_put(dpmacs);
> + return child;
> + }
...
> + if (is_of_node(dev->parent->fwnode))
Ditto ?
> + of_node_put(dpmacs);
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v5 10/15] net: mdio: Add ACPI support code for mdio
On Mon, Feb 8, 2021 at 5:14 PM Calvin Johnson
wrote:
>
> Define acpi_mdiobus_register() to Register mii_bus and create PHYs for
> each ACPI child node.
...
> +/**
> + * acpi_mdiobus_register - Register mii_bus and create PHYs from the ACPI
> ASL.
> + *
Redundant blank line.
> + * @mdio: pointer to mii_bus structure
> + * @fwnode: pointer to fwnode of MDIO bus.
> + *
> + * This function registers the mii_bus structure and registers a phy_device
> + * for each child node of @fwnode.
> + */
> +int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> +{
> + struct fwnode_handle *child;
> + u32 addr;
> + int ret;
> +
> + /* Mask out all PHYs from auto probing. */
> + mdio->phy_mask = ~0;
I would rather see GENMASK(31, 0) here because in case the type of the
variable is changed we will need to amend this anyway.
> + ret = mdiobus_register(mdio);
> + if (ret)
> + return ret;
> + mdio->dev.fwnode = fwnode;
Shouldn't it be rather ACPI_SET_COMPANION() as other bus / drivers do?
> +/* Loop over the child nodes and register a phy_device for each PHY */
Indentation.
> + fwnode_for_each_child_node(fwnode, child) {
> + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child),
> &addr);
> + if ((ret) || addr >= PHY_MAX_ADDR)
Too many parentheses.
> + continue;
> +
> + ret = fwnode_mdiobus_register_phy(mdio, child, addr);
> + if (ret == -ENODEV)
> + dev_err(&mdio->dev,
> + "MDIO device at address %d is missing.\n",
> + addr);
> + }
> + return 0;
> +}
...
> +/*
> + * ACPI helpers for the MDIO (Ethernet PHY) API
> + *
> + */
It's one line AFAICT!
...
> +#include
> +#include
This seems a bit inconsistent with the below.
I see the user of mdiobus_register(). It's the only header should be
included. Everything else would be forward declared like
struct fwnode_handle;
> +#if IS_ENABLED(CONFIG_ACPI_MDIO)
> +int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle
> *fwnode);
> +#else /* CONFIG_ACPI_MDIO */
> +static inline int acpi_mdiobus_register(struct mii_bus *mdio, struct
> fwnode_handle *fwnode)
> +{
> + /*
> +* Fall back to mdiobus_register() function to register a bus.
> +* This way, we don't have to keep compat bits around in drivers.
> +*/
> +
> + return mdiobus_register(mdio);
> +}
> +#endif
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v4 07/15] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
On Fri, Feb 5, 2021 at 8:41 PM Andy Shevchenko
wrote:
> On Fri, Feb 5, 2021 at 8:25 PM Andy Shevchenko
> wrote:
> > On Fri, Feb 5, 2021 at 7:25 PM Calvin Johnson
> > wrote:
> > > On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote:
> >
> > ...
> >
> > > > + rc = fwnode_property_match_string(child, "compatible",
> > > > "ethernet-phy-ieee802.3-c45");
> > > With ACPI, I'm facing some problem with fwnode_property_match_string().
> > > It is
> > > unable to detect the compatible string and returns -EPROTO.
> > >
> > > ACPI node for PHY4 is as below:
> > >
> > > Device(PHY4) {
> > > Name (_ADR, 0x4)
> > > Name(_CRS, ResourceTemplate() {
> > > Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> > > {
> > > AQR_PHY4_IT
> > > }
> > > }) // end of _CRS for PHY4
> > > Name (_DSD, Package () {
> > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > Package () {
>
> > > Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
>
> I guess converting this to
>Package () {"compatible", Package() {"ethernet-phy-ieee802.3-c45"}}
> will solve it.
For the record, it doesn't mean there is no bug in the code. DT treats
a single string as an array, but ACPI doesn't.
And this is specific to _match_string() because it has two passes. And
the first one fails.
While reading a single string as an array of 1 element will work I believe.
> > >}
>
> > > })
> > > } // end of PHY4
> > >
> > > What is see is that in acpi_data_get_property(),
> > > propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE).
> > >
> > > Any help please?
> > >
> > > fwnode_property_match_string() works fine for DT.
> >
> > Can you show the DT node which works and also input for the
> > )match_string() (i.o.w what exactly you are trying to match with)?
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v4 07/15] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
On Fri, Feb 5, 2021 at 8:25 PM Andy Shevchenko
wrote:
> On Fri, Feb 5, 2021 at 7:25 PM Calvin Johnson
> wrote:
> > On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote:
>
> ...
>
> > > + rc = fwnode_property_match_string(child, "compatible",
> > > "ethernet-phy-ieee802.3-c45");
> > With ACPI, I'm facing some problem with fwnode_property_match_string(). It
> > is
> > unable to detect the compatible string and returns -EPROTO.
> >
> > ACPI node for PHY4 is as below:
> >
> > Device(PHY4) {
> > Name (_ADR, 0x4)
> > Name(_CRS, ResourceTemplate() {
> > Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> > {
> > AQR_PHY4_IT
> > }
> > }) // end of _CRS for PHY4
> > Name (_DSD, Package () {
> > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > Package () {
> > Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
I guess converting this to
Package () {"compatible", Package() {"ethernet-phy-ieee802.3-c45"}}
will solve it.
> >}
> > })
> > } // end of PHY4
> >
> > What is see is that in acpi_data_get_property(),
> > propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE).
> >
> > Any help please?
> >
> > fwnode_property_match_string() works fine for DT.
>
> Can you show the DT node which works and also input for the
> )match_string() (i.o.w what exactly you are trying to match with)?
>
> --
> With Best Regards,
> Andy Shevchenko
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v4 07/15] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
On Fri, Feb 5, 2021 at 7:25 PM Calvin Johnson
wrote:
> On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote:
...
> > + rc = fwnode_property_match_string(child, "compatible",
> > "ethernet-phy-ieee802.3-c45");
> With ACPI, I'm facing some problem with fwnode_property_match_string(). It is
> unable to detect the compatible string and returns -EPROTO.
>
> ACPI node for PHY4 is as below:
>
> Device(PHY4) {
> Name (_ADR, 0x4)
> Name(_CRS, ResourceTemplate() {
> Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> {
> AQR_PHY4_IT
> }
> }) // end of _CRS for PHY4
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
>}
> })
> } // end of PHY4
>
> What is see is that in acpi_data_get_property(),
> propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE).
>
> Any help please?
>
> fwnode_property_match_string() works fine for DT.
Can you show the DT node which works and also input for the
)match_string() (i.o.w what exactly you are trying to match with)?
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v4 01/15] Documentation: ACPI: DSD: Document MDIO PHY
On Fri, Jan 29, 2021 at 6:44 PM Rafael J. Wysocki wrote:
> On Fri, Jan 29, 2021 at 5:37 PM Rafael J. Wysocki wrote:
> > On Fri, Jan 29, 2021 at 7:48 AM Calvin Johnson
> > wrote:
...
> > It would work, but I would introduce a wrapper around the _ADR
> > evaluation, something like:
> >
> > int acpi_get_local_address(acpi_handle handle, u32 *addr)
> > {
> > unsigned long long adr;
> > acpi_status status;
> >
> > status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
> > if (ACPI_FAILURE(status))
> > return -ENODATA;
> >
> > *addr = (u32)adr;
> > return 0;
> > }
> >
> > in drivers/acpi/utils.c and add a static inline stub always returning
> > -ENODEV for it for !CONFIG_ACPI.
...
> BTW, you may not need the fwnode_get_local_addr() at all then, just
> evaluate either the "reg" property for OF or acpi_get_local_address()
> for ACPI in the "caller" code directly. A common helper doing this can
> be added later.
Sounds good to me and it will address your concern about different
semantics of reg/_ADR on per driver/subsystem basis.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/1] net: dsa: rtl8366rb: standardize init jam tables
On Mon, Jan 25, 2021 at 7:00 AM Lorenzo Carletti
wrote:
>
> In the rtl8366rb driver there are some jam tables which contain
> undocumented values.
> While trying to understand what these tables actually do,
> I noticed a discrepancy in how one of those was treated.
> Most of them were plain u16 arrays, while the ethernet one was
> an u16 matrix.
> By looking at the vendor's droplets of source code these tables came from,
> I found out that they were all originally u16 matrixes.
>
> This commit standardizes the jam tables, turning them all into
> u16 matrixes.
> This change makes it easier to understand how the jam tables are used
> and also makes it possible for a single function to handle all of them,
> removing some duplicated code.
...
Since further replies removed code, I reply here for everybody with an
example of my thoughts against this cryptic data.
If you look into the below, for example, you may notice a few things.
- it pokes different address regions (sounds like data section, text
section, etc.)
- it has different meaning for some addresses (0xBE prefix)
+static const u16 rtl8366rb_init_jam_f5d8235[][2] = {
+ {0x0242, 0x02BF}, {0x0245, 0x02BF}, {0x0248, 0x02BF}, {0x024B, 0x02BF},
+ {0x024E, 0x02BF}, {0x0251, 0x02BF}, {0x0254, 0x0A3F}, {0x0256, 0x0A3F},
+ {0x0258, 0x0A3F}, {0x025A, 0x0A3F}, {0x025C, 0x0A3F}, {0x025E, 0x0A3F},
Sounds like we program some buffer lengths / limits (0x2c0, 0xa40 if
it rings any bell to anybody).
+ {0x0263, 0x007C}, {0x0100, 0x0004}, {0xBE5B, 0x3500}, {0x800E, 0x200F},
BE5B seems like "execute the routine at 0x3500 address".
Thus I think shuffling those pairs before 0xbe shouldn't give any
difference (but I have no hw to try).
+ {0xBE1D, 0x0F00}, {0x8001, 0x5011}, {0x800A, 0xA2F4}, {0x800B, 0x17A3},
+ {0xBE4B, 0x17A3}, {0xBE41, 0x5011}, {0xBE17, 0x2100}, {0x8000, 0x8304},
+ {0xBE40, 0x8304}, {0xBE4A, 0xA2F4}, {0x800C, 0xA8D5}, {0x8014, 0x5500},
+ {0x8015, 0x0004}, {0xBE4C, 0xA8D5}, {0xBE59, 0x0008}, {0xBE09, 0x0E00},
+ {0xBE36, 0x1036}, {0xBE37, 0x1036}, {0x800D, 0x00FF}, {0xBE4D, 0x00FF},
0x80 addresses are some kind of magic, like interrupt vector returns
or so. You may notice some 0xBE commands against the addresses that
are put into the 0x8000 address region.
};
Overall it seems you have to discover a full firmware image to make
any assumptions about CPU ISA used there and address mapping.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()
On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan wrote: > On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki wrote: > > On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan > > wrote: > > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki > > > wrote: > > > I'd rather this new function be an ops instead of a bunch of #ifdef or > > > if (acpi) checks. Thoughts? > > > > Well, it looks more like a helper function than like an op and I'm not > > even sure how many potential users of it will expect that _ADR should > > be evaluated in the absence of the "reg" property. > > > > It's just that the "reg" property happens to be kind of an _ADR > > equivalent in this particular binding AFAICS. > > I agree it is not clear how useful this helper function is going to be. > > But in general, to me, any time the wrapper/helper functions in > drivers/base/property.c need to do something like this: > > if (ACPI) >ACPI specific code > if (OF) >OF specific code > > I think the code should be pushed to the fwnode ops. That's one of the > main point of fwnode. So that firmware specific stuff is done by > firmware specific code. Also, when adding support for new firmware, > it's pretty clear what support the firmware needs to implement. > Instead of having to go fix up a bunch of code all over the place. Wishful thinking. In the very case of GPIO it's related to framework using headers local to framework. Are you suggesting to open its guts to the entire wild world? I don't think it's a good idea. You see, here we have different layering POD types, which are natural and quite low level that ops suits best for them and quite different resource types like GPIO. And the latter is closer to certain framework rather than to POD handling cases. > So fwnode_ops->get_id() would be the OP ACPI and OF would implement. > And then we can have a wrapper in drivers/base/property.c. -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v4 09/15] device property: Introduce fwnode_get_id()
On Fri, Jan 22, 2021 at 05:40:41PM +0100, Rafael J. Wysocki wrote: > On Fri, Jan 22, 2021 at 4:46 PM Calvin Johnson > wrote: > > > > Using fwnode_get_id(), get the reg property value for DT node > > or get the _ADR object value for ACPI node. > > So I'm not really sure if this is going to be generically useful. > > First of all, the meaning of the _ADR return value is specific to a > given bus type (e.g. the PCI encoding of it is different from the I2C > encoding of it) and it just happens to be matching the definition of > the "reg" property for this particular binding. > IOW, not everyone may expect the "reg" property and the _ADR return > value to have the same encoding and belong to the same set of values, I have counted three or even four attempts to open code exact this scenario in the past couple of years. And I have no idea where to put a common base for them so they will not duplicate this in each case. > so maybe put this function somewhere closer to the code that's going > to use it, because it seems to be kind of specific to this particular > use case? -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v4 09/15] device property: Introduce fwnode_get_id()
On Fri, Jan 22, 2021 at 09:12:54PM +0530, Calvin Johnson wrote:
> Using fwnode_get_id(), get the reg property value for DT node
> or get the _ADR object value for ACPI node.
...
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * This function provides the id of a fwnode which can be either
> + * DT or ACPI node. For ACPI, "reg" property value, if present will
> + * be provided or else _ADR value will be provided.
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +#ifdef CONFIG_ACPI
> + unsigned long long adr;
> + acpi_status status;
> +#endif
Instead you may do...
> + int ret;
> +
> + ret = fwnode_property_read_u32(fwnode, "reg", id);
> + if (ret) {
> +#ifdef CONFIG_ACPI
...it here like
unsigned long long adr;
acpi_status status;
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> +METHOD_NAME__ADR, NULL, &adr);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> + *id = (u32)adr;
> +#else
> + return ret;
> +#endif
> + }
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki wrote:
> On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
> wrote:
> > On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
> > wrote:
...
> > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > > +{
> > > +#ifdef CONFIG_ACPI
> > > + unsigned long long adr;
> > > + acpi_status status;
> > > +#endif
> > > + int ret;
> > > +
> > > + ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > + if (!(ret && is_acpi_node(fwnode)))
> > > + return ret;
> > > +
> > > +#ifdef CONFIG_ACPI
> > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > + METHOD_NAME__ADR, NULL, &adr);
> > > + if (ACPI_FAILURE(status))
> > > + return -EINVAL;
> > > + *id = (u32)adr;
> >
> > Shouldn't be
> >
> >return 0;
> > #else
> >return -EINVAL;
> > #endif
> >
> > ?
> >
> > Yes, it's a theoretical case when is_acpi_node() returns true when
> > CONFIG_ACPI=n.
>
> How so? is_acpi_node() is defined as a static inline returning false then.
I understand that, that's why it's pure theoretical when, for example,
the semantics is changed. But I believe it's unlucky to happen.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki wrote:
> On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> wrote:
> > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > wrote:
...
> > > > + ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > + if (!(ret && is_acpi_node(fwnode)))
> > > > + return ret;
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > + METHOD_NAME__ADR, NULL, &adr);
> > > > + if (ACPI_FAILURE(status))
> > > > + return -EINVAL;
> > > > + *id = (u32)adr;
> > > > +#endif
> > > > + return 0;
> > > Also ACPI and DT
> > > aren't mutually exclusive if I'm not mistaken.
> >
> > That's why we try 'reg' property for both cases first.
> >
> > is_acpi_fwnode() conditional is that what I don't like though.
>
> I'm not sure what you mean here, care to elaborate?
I meant is_acpi_node(fwnode) in the conditional.
I think it's redundant and we can simple do something like this:
if (ret) {
#ifdef ACPI
...
#else
return ret;
#endif
}
return 0;
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.
...
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > + unsigned long long adr;
> > + acpi_status status;
> > +#endif
> > + int ret;
> > +
> > + ret = fwnode_property_read_u32(fwnode, "reg", id);
> > + if (!(ret && is_acpi_node(fwnode)))
> > + return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > + METHOD_NAME__ADR, NULL, &adr);
> > + if (ACPI_FAILURE(status))
> > + return -EINVAL;
> > + *id = (u32)adr;
> > +#endif
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_get_id);
> Please don't do it this way. The whole point of fwnode_operations is
> to avoid conditional stuff at the fwnode level.
Not fully true. We have non-POD getters that are conditional. Moreover,
we have additional layer of Primary / Secondary fwnodes on top of that.
The caller of fwnode API is indeed agnostic, but under the hood it differs by
the definition (obviously due to natural differences between ACPI and DT and
whatever else might come in the future.
> Also ACPI and DT
> aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
...
> fwnode is lower level that the device-driver framework.
Agree.
> Making
> it aware of busses like mdio, etc doesn't sound right.
Disagree. Conceptually resource providers can be quite different and fwnode API
*is* LCM for them.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v3 14/15] net: phylink: Refactor phylink_of_phy_connect()
On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson wrote: > > Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect(). Same Q as per previous patch. If it's indeed a bug in the existing code, should be fixed in a separate patch -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v3 13/15] phylink: introduce phylink_fwnode_phy_connect()
On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson wrote: > > Define phylink_fwnode_phy_connect() to connect phy specified by > a fwnode to a phylink instance. ... > + phy_dev = fwnode_phy_find_device(phy_fwnode); > + /* We're done with the phy_node handle */ > + fwnode_handle_put(phy_fwnode); > + if (!phy_dev) > + return -ENODEV; > + > + ret = phy_attach_direct(pl->netdev, phy_dev, flags, > + pl->link_interface); > + if (ret) Hmm... Shouldn't you put phy_dev here? > + return ret; -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v3 12/15] net/fsl: Use fwnode_mdiobus_register()
On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson wrote: > > fwnode_mdiobus_register() internally takes care of both DT > and ACPI cases to register mdiobus. Replace existing > of_mdiobus_register() with fwnode_mdiobus_register(). > > Note: For both ACPI and DT cases, endianness of MDIO controller > need to be specified using "little-endian" property. > + /* For both ACPI and DT cases, endianness of MDIO controller > +* need to be specified using "little-endian" property. needs > +*/ ... > priv->has_a011043 = device_property_read_bool(&pdev->dev, > "fsl,erratum-a011043"); > - Unrelated change. -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v3 11/15] net: mdiobus: Introduce fwnode_mdiobus_register()
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
wrote:
>
> Introduce fwnode_mdiobus_register() to register PHYs on the mdiobus.
> If the fwnode is DT node, then call of_mdiobus_register().
> If it is an ACPI node, then call acpi_mdiobus_register().
...
> +/**
> + * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
> + * @mdio: pointer to mii_bus structure
> + * @fwnode: pointer to fwnode of MDIO bus.
> + *
> + * This function returns of_mdiobus_register() for DT and
> + * acpi_mdiobus_register() for ACPI.
> + */
> +int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle
> *fwnode)
> +{
> + if (is_of_node(fwnode))
> + return of_mdiobus_register(mdio, to_of_node(fwnode));
> + else if (is_acpi_node(fwnode))
Redundant 'else'
> + return acpi_mdiobus_register(mdio, fwnode);
> +
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> or get the _ADR object value for ACPI node.
>
> Signed-off-by: Calvin Johnson
> ---
>
> Changes in v3:
> - Modified to retrieve reg property value for ACPI as well
> - Resolved compilation issue with CONFIG_ACPI = n
> - Added more info into documentation
>
> Changes in v2: None
>
> drivers/base/property.c | 33 +
> include/linux/property.h | 1 +
> 2 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 35b95c6ac0c6..2d51108cb936 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct
> fwnode_handle *fwnode)
> return fwnode_call_ptr_op(fwnode, get_name_prefix);
> }
>
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * This function provides the id of a fwnode which can be either
> + * DT or ACPI node. For ACPI, "reg" property value, if present will
> + * be provided or else _ADR value will be provided.
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +#ifdef CONFIG_ACPI
> + unsigned long long adr;
> + acpi_status status;
> +#endif
> + int ret;
> +
> + ret = fwnode_property_read_u32(fwnode, "reg", id);
> + if (!(ret && is_acpi_node(fwnode)))
> + return ret;
> +
> +#ifdef CONFIG_ACPI
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> + METHOD_NAME__ADR, NULL, &adr);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> + *id = (u32)adr;
Shouldn't be
return 0;
#else
return -EINVAL;
#endif
?
Yes, it's a theoretical case when is_acpi_node() returns true when
CONFIG_ACPI=n.
> +#endif
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_id);
> +
> /**
> * fwnode_get_parent - Return parent firwmare node
> * @fwnode: Firmware whose parent is retrieved
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 0a9001fe7aea..3f41475f010b 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct
> fwnode_handle *fwnode,
>
> const char *fwnode_get_name(const struct fwnode_handle *fwnode);
> const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
> struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
> struct fwnode_handle *fwnode_get_next_parent(
> struct fwnode_handle *fwnode);
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/1] time64.h: Consolidated PSEC_PER_SEC definition
We have currently three users of the PSEC_PER_SEC each of them defining it
individually. Instead, move it to time64.h to be available for everyone.
There is a new user coming with the same constant in use. It will also
make its life easier.
Signed-off-by: Andy Shevchenko
---
drivers/net/ethernet/mscc/ocelot_ptp.c | 2 ++
drivers/phy/phy-core-mipi-dphy.c | 2 --
drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c | 8
include/soc/mscc/ocelot_ptp.h| 2 --
include/vdso/time64.h| 1 +
5 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_ptp.c
b/drivers/net/ethernet/mscc/ocelot_ptp.c
index a33ab315cc6b..87ad2137ba06 100644
--- a/drivers/net/ethernet/mscc/ocelot_ptp.c
+++ b/drivers/net/ethernet/mscc/ocelot_ptp.c
@@ -4,6 +4,8 @@
* Copyright (c) 2017 Microsemi Corporation
* Copyright 2020 NXP
*/
+#include
+
#include
#include
#include
diff --git a/drivers/phy/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
index 14e0551cd319..77fe65367ce5 100644
--- a/drivers/phy/phy-core-mipi-dphy.c
+++ b/drivers/phy/phy-core-mipi-dphy.c
@@ -12,8 +12,6 @@
#include
#include
-#define PSEC_PER_SEC 1LL
-
/*
* Minimum D-PHY timings based on MIPI D-PHY specification. Derived
* from the valid ranges specified in Section 6.9, Table 14, Page 41
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
index 8af8c6c5cc02..347dc79a18c1 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
@@ -11,16 +11,16 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
+#include
+
#include
#include
-#include
-#include
-
-#define PSEC_PER_SEC 1LL
#define UPDATE(x, h, l)(((x) << (l)) & GENMASK((h), (l)))
diff --git a/include/soc/mscc/ocelot_ptp.h b/include/soc/mscc/ocelot_ptp.h
index 6a7388fa7cc5..ded497d72bdb 100644
--- a/include/soc/mscc/ocelot_ptp.h
+++ b/include/soc/mscc/ocelot_ptp.h
@@ -37,8 +37,6 @@ enum {
#define PTP_CFG_MISC_PTP_ENBIT(2)
-#define PSEC_PER_SEC 1LL
-
#define PTP_CFG_CLK_ADJ_CFG_ENABIT(0)
#define PTP_CFG_CLK_ADJ_CFG_DIRBIT(1)
diff --git a/include/vdso/time64.h b/include/vdso/time64.h
index 9d43c3f5e89d..b40cfa2aa33c 100644
--- a/include/vdso/time64.h
+++ b/include/vdso/time64.h
@@ -9,6 +9,7 @@
#define NSEC_PER_MSEC 100L
#define USEC_PER_SEC 100L
#define NSEC_PER_SEC 10L
+#define PSEC_PER_SEC 1LL
#define FSEC_PER_SEC 1000LL
#endif /* __VDSO_TIME64_H */
--
2.29.2
Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()
On Fri, Dec 18, 2020 at 7:40 AM Calvin Johnson wrote: > On Tue, Dec 15, 2020 at 07:53:26PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson > > wrote: ... > > I would rather see this as simple as > > > > if (is_of_node(fwnode)) > >return of_mdiobus_register(mdio, to_of_node(fwnode)); > > if (is_acpi_node(fwnode)) > >return acpi_mdiobus_register(mdio, fwnode); > > > > where the latter one is defined somewhere in drivers/acpi/. > Makes sense. I'll do it. But I think it will be better to place > acpi_mdiobus_register() here itself in the network subsystem, maybe > /drivers/net/mdio/acpi_mdio.c. Even better, thanks! -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
On Fri, Dec 18, 2020 at 7:34 AM Calvin Johnson wrote: > > On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson > > wrote: ... > > > + /* phy->mii_ts may already be defined by the PHY driver. A > > > +* mii_timestamper probed via the device tree will still > > > have > > > +* precedence. > > > +*/ > > > > > + if (mii_ts) > > > + phy->mii_ts = mii_ts; > > > > How is that defined? Do you need to do something with an old pointer? > > As the comment says, I think PHY drivers which got invoked before calling > of_mdiobus_register_phy() may have defined phy->mii_ts. What I meant here is that the old pointer might need some care (freeing?). > > > + } -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()
On Thu, Dec 17, 2020 at 10:28 AM Calvin Johnson
wrote:
> On Tue, Dec 15, 2020 at 07:28:10PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> > wrote:
...
> > > + if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> >*phy_id = ((upper & 0x) << 16) | (lower & 0x);
> > And perhaps GENMASK() ?
>
> Sure. Will rewrite accordingly.
Reading this again I'm now not sure these masks are needed at all.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v2 09/14] net/fsl: Use fwnode_mdiobus_register()
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson wrote: > > fwnode_mdiobus_register() internally takes care of both DT > and ACPI cases to register mdiobus. Replace existing > of_mdiobus_register() with fwnode_mdiobus_register(). > > Note: For both ACPI and DT cases, endianness of MDIO controller > need to be specified using "little-endian" property. ... > @@ -2,6 +2,7 @@ > * QorIQ 10G MDIO Controller > * > * Copyright 2012 Freescale Semiconductor, Inc. > + * Copyright 2020 NXP > * > * Authors: Andy Fleming > * Timur Tabi > @@ -11,6 +12,7 @@ I guess this... > priv->is_little_endian = device_property_read_bool(&pdev->dev, >"little-endian"); > - > priv->has_a011043 = device_property_read_bool(&pdev->dev, > "fsl,erratum-a011043"); ...this... > - ...and this changes can go to a separate patch. -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
wrote:
>
> Introduce fwnode_mdiobus_register() to register PHYs on the mdiobus.
> If the fwnode is DT node, then call of_mdiobus_register().
> If it is an ACPI node, then:
> - disable auto probing of mdiobus
> - register mdiobus
> - save fwnode to mdio structure
> - loop over child nodes & register a phy_device for each PHY
...
> +/**
> + * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
> + * @mdio: pointer to mii_bus structure
> + * @fwnode: pointer to fwnode of MDIO bus.
> + *
> + * This function registers the mii_bus structure and registers a phy_device
> + * for each child node of @fwnode.
> + */
> +int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle
> *fwnode)
> +{
> + struct fwnode_handle *child;
> + unsigned long long addr;
> + acpi_status status;
> + int ret;
> +
> + if (is_of_node(fwnode)) {
> + return of_mdiobus_register(mdio, to_of_node(fwnode));
> + } else if (is_acpi_node(fwnode)) {
I would rather see this as simple as
if (is_of_node(fwnode))
return of_mdiobus_register(mdio, to_of_node(fwnode));
if (is_acpi_node(fwnode))
return acpi_mdiobus_register(mdio, fwnode);
where the latter one is defined somewhere in drivers/acpi/.
> + /* Mask out all PHYs from auto probing. */
> + mdio->phy_mask = ~0;
> + ret = mdiobus_register(mdio);
> + if (ret)
> + return ret;
> +
> + mdio->dev.fwnode = fwnode;
> + /* Loop over the child nodes and register a phy_device for each PHY */
> + fwnode_for_each_child_node(fwnode, child) {
> + status =
> acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> + "_ADR", NULL, &addr);
> + if (ACPI_FAILURE(status)) {
Isn't it fwnode_get_id() now?
> + pr_debug("_ADR returned %d\n", status);
> + continue;
> + }
> + if (addr < 0 || addr >= PHY_MAX_ADDR)
> + continue;
addr can't be less than 0.
> + ret = fwnode_mdiobus_register_phy(mdio, child, addr);
> + if (ret == -ENODEV)
> + dev_err(&mdio->dev,
> + "MDIO device at address %lld is
> missing.\n",
> + addr);
> + }
> + return 0;
> + }
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()
On Tue, Dec 15, 2020 at 7:00 PM Laurent Pinchart
wrote:
> On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote:
> > Using fwnode_get_id(), get the reg property value for DT node
> > and get the _ADR object value for ACPI node.
...
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
>
> Is the concept of fwnode ID documented clearly somewhere ? I think this
> function should otherwise have more documentation, at least to explain
> what the ID is.
I'm afraid that OF has no clear concept of this either. It's usually
used as a unique ID for the children of some device (like MFD) and can
represent a lot of things. But I agree it should be documented.
Rob, any ideas about this?
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > + unsigned long long adr;
> > + acpi_status status;
> > +
> > + if (is_of_node(fwnode)) {
> > + return of_property_read_u32(to_of_node(fwnode), "reg", id);
> > + } else if (is_acpi_node(fwnode)) {
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +METHOD_NAME__ADR, NULL, &adr);
> > + if (ACPI_FAILURE(status))
> > + return -ENODATA;
>
> Would it make sense to standardize error codes ? of_property_read_u32()
> can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of
> this function would be very interested to tell those three cases apart.
> Maybe we should return -EINVAL in all error cases ? Or maybe different
> error codes to mean "the backend doesn't support the concept of IDs",
> and "the device doesn't have an ID" ?
We may actually get mapping to all three if first we check for the
method/name existence followed by value check.
But I don't think we need to bloat this simple one.
> > + *id = (u32)adr;
> > + return 0;
> > + }
> > + return -EINVAL;
> > +}
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> and get the _ADR object value for ACPI node.
and -> or
...
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> + unsigned long long adr;
> + acpi_status status;
> +
> + if (is_of_node(fwnode)) {
> + return of_property_read_u32(to_of_node(fwnode), "reg", id);
ACPI nodes can hold reg property as well. I would rather think about
ret = fwnode_property_read_u32(fwnode, "reg", id)
if (!(ret && is_acpi_node(fwnode)))
return ret;
> + } else if (is_acpi_node(fwnode)) {
Redundant 'else'
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> + METHOD_NAME__ADR, NULL, &adr);
> + if (ACPI_FAILURE(status))
> + return -ENODATA;
I'm wondering if it compiles when CONFIG_ACPI=n.
> + *id = (u32)adr;
> + return 0;
> + }
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
wrote:
>
> Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> mdiobus. From the compatible string, identify whether the PHY is
> c45 and based on this create a PHY device instance which is
> registered on the mdiobus.
...
> +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> + struct fwnode_handle *child, u32 addr)
> +{
> + struct mii_timestamper *mii_ts;
> + struct phy_device *phy;
> + const char *cp;
> + bool is_c45;
> + u32 phy_id;
> + int rc;
> + if (is_of_node(child)) {
> + mii_ts = of_find_mii_timestamper(to_of_node(child));
> + if (IS_ERR(mii_ts))
> + return PTR_ERR(mii_ts);
> + }
Perhaps
mii_ts = of_find_mii_timestamper(to_of_node(child));
> +
> + rc = fwnode_property_read_string(child, "compatible", &cp);
> + is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
> +
> + if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> + phy = get_phy_device(bus, addr, is_c45);
> + else
> + phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> + if (IS_ERR(phy)) {
> + if (mii_ts && is_of_node(child))
> + unregister_mii_timestamper(mii_ts);
if (!IS_ERR_OR_NULL(mii_ts))
...
However it points to the question why unregister() doesn't handle the
above cases.
I would expect unconditional call to unregister() here.
> + return PTR_ERR(phy);
> + }
> +
> + if (is_acpi_node(child)) {
> + phy->irq = bus->irq[addr];
> +
> + /* Associate the fwnode with the device structure so it
> +* can be looked up later.
> +*/
> + phy->mdio.dev.fwnode = child;
> +
> + /* All data is now stored in the phy struct, so register it */
> + rc = phy_device_register(phy);
> + if (rc) {
> + phy_device_free(phy);
> + fwnode_handle_put(phy->mdio.dev.fwnode);
> + return rc;
> + }
> +
> + dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> + } else if (is_of_node(child)) {
> + rc = of_mdiobus_phy_device_register(bus, phy,
> to_of_node(child), addr);
> + if (rc) {
> + if (mii_ts)
> + unregister_mii_timestamper(mii_ts);
Ditto.
> + phy_device_free(phy);
> + return rc;
> + }
> +
> + /* phy->mii_ts may already be defined by the PHY driver. A
> +* mii_timestamper probed via the device tree will still have
> + * precedence.
> +*/
> + if (mii_ts)
> + phy->mii_ts = mii_ts;
How is that defined? Do you need to do something with an old pointer?
> + }
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
wrote:
>
> Extract phy_id from compatible string. This will be used by
> fwnode_mdiobus_register_phy() to create phy device using the
> phy_id.
...
> + if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> + *phy_id = ((upper & 0x) << 16) | (lower & 0x);
> + return 0;
> + }
> + return -EINVAL;
Perhaps traditional pattern, i.e.
if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) != 2)
return -EINVAL;
*phy_id = ((upper & 0x) << 16) | (lower & 0x);
return 0;
And perhaps GENMASK() ?
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v2 02/14] net: phy: Introduce phy related fwnode functions
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
wrote:
>
> Define fwnode_phy_find_device() to iterate an mdiobus and find the
> phy device of the provided phy fwnode. Additionally define
> device_phy_find_device() to find phy device of provided device.
>
> Define fwnode_get_phy_node() to get phy_node using named reference.
...
> +#include
Not sure we need this. See below.
...
> +/**
> + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
> + * phy_fwnode.
Can we keep a summary on one line?
> + * @phy_fwnode: Pointer to the phy's fwnode.
> + *
> + * If successful, returns a pointer to the phy_device with the embedded
> + * struct device refcount incremented by one, or NULL on failure.
> + */
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> + struct mdio_device *mdiodev;
> + struct device *d;
> + if (!phy_fwnode)
> + return NULL;
Why is this needed?
Perhaps a comment to the function description explains a case when
@phy_fwnode == NULL.
> + d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
> + if (d) {
> + mdiodev = to_mdio_device(d);
> + if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> + return to_phy_device(d);
> + put_device(d);
> + }
> +
> + return NULL;
> +}
...
> + * For ACPI, only "phy-handle" is supported. DT supports all the three
> + * named references to the phy node.
...
> + /* Only phy-handle is used for ACPI */
> + phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> + if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> + return phy_node;
So, what is the problem with going through the rest on ACPI?
Usually we describe the restrictions in the documentation.
--
With Best Regards,
Andy Shevchenko
Re: [patch 03/30] genirq: Move irq_set_lockdep_class() to core
On Sat, Dec 12, 2020 at 12:07 AM Thomas Gleixner wrote:
>
> On Fri, Dec 11 2020 at 22:08, Thomas Gleixner wrote:
>
> > On Fri, Dec 11 2020 at 19:53, Andy Shevchenko wrote:
> >
> >> On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner
> >> wrote:
> >>>
> >>> irq_set_lockdep_class() is used from modules and requires irq_to_desc() to
> >>> be exported. Move it into the core code which lifts another requirement
> >>> for
> >>> the export.
> >>
> >> ...
> >>
> >>> + if (IS_ENABLED(CONFIG_LOCKDEP))
> >>> + __irq_set_lockdep_class(irq, lock_class, request_class);
> >
> > You are right. Let me fix that.
>
> No. I have to correct myself. You're wrong.
>
> The inline is evaluated in the compilation units which include that
> header and because the function declaration is unconditional it is
> happy.
>
> Now the optimizer stage makes the whole thing a NOOP if CONFIG_LOCKDEP=n
> and thereby drops the reference to the function which makes it not
> required for linking.
>
> So in the file where the function is implemented:
>
> #ifdef CONFIG_LOCKDEP
> void __irq_set_lockdep_class()
> {
> }
> #endif
>
> The whole block is either discarded because CONFIG_LOCKDEP is not
> defined or compile if it is defined which makes it available for the
> linker.
>
> And in the latter case the optimizer keeps the call in the inline (it
> optimizes the condition away because it's always true).
>
> So in both cases the compiler and the linker are happy and everything
> works as expected.
>
> It would fail if the header file had the following:
>
> #ifdef CONFIG_LOCKDEP
> void __irq_set_lockdep_class();
> #endif
>
> Because then it would complain about the missing function prototype when
> it evaluates the inline.
I understand that (that's why I put "if even no warning") and what I'm
talking about is the purpose of IS_ENABLED(). It's usually good for
compile testing !CONFIG_FOO cases. But here it seems inconsistent.
The pattern I usually see in the cases like this is
#ifdef CONFIG_LOCKDEP
void __irq_set_lockdep_class();
#else
static inline void ... {}
#endif
and call it directly in the caller.
It's not a big deal, so up to you.
--
With Best Regards,
Andy Shevchenko
Re: [patch 16/30] mfd: ab8500-debugfs: Remove the racy fiddling with irq_desc
On Thu, Dec 10, 2020 at 9:57 PM Thomas Gleixner wrote: > > First of all drivers have absolutely no business to dig into the internals > of an irq descriptor. That's core code and subject to change. All of this > information is readily available to /proc/interrupts in a safe and race > free way. > > Remove the inspection code which is a blatant violation of subsystem > boundaries and racy against concurrent modifications of the interrupt > descriptor. > > Print the irq line instead so the information can be looked up in a sane > way in /proc/interrupts. ... > - seq_printf(s, "%3i: %6i %4i", > + seq_printf(s, "%3i: %6i %4i %4i\n", Seems different specifiers, I think the intention was something like seq_printf(s, "%3i: %4i %6i %4i\n", >line, > + line + irq_first, >num_interrupts[line], > num_wake_interrupts[line]); -- With Best Regards, Andy Shevchenko
Re: [patch 03/30] genirq: Move irq_set_lockdep_class() to core
On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner wrote: > > irq_set_lockdep_class() is used from modules and requires irq_to_desc() to > be exported. Move it into the core code which lifts another requirement for > the export. ... > + if (IS_ENABLED(CONFIG_LOCKDEP)) > + __irq_set_lockdep_class(irq, lock_class, request_class); Maybe I missed something, but even if the compiler does not warn the use of if IS_ENABLED() with complimentary #ifdef seems inconsistent. > +#ifdef CONFIG_LOCKDEP ... > +EXPORT_SYMBOL_GPL(irq_set_lockdep_class); > +#endif -- With Best Regards, Andy Shevchenko
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Wed, Dec 9, 2020 at 12:59 PM Andy Shevchenko
wrote:
> On Wed, Dec 9, 2020 at 10:35 AM Heiner Kallweit wrote:
...
> > -int pci_try_set_mwi(struct pci_dev *dev)
> > -{
>
> > -#ifdef PCI_DISABLE_MWI
> > - return 0;
> > -#else
> > - return pci_set_mwi(dev);
> > -#endif
>
> This seems still valid case for PowerPC and SH.
I see that pci_set_mwi() also has the ifdeffery (I thought it's only
here), so it's fine.
> > -}
> > -EXPORT_SYMBOL(pci_try_set_mwi);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] PCI: Remove pci_try_set_mwi
On Wed, Dec 9, 2020 at 10:35 AM Heiner Kallweit wrote:
> pci_set_mwi() and pci_try_set_mwi() do exactly the same, just that the
> former one is declared as __must_check. However also some callers of
However also -> However
> pci_set_mwi() have a comment that it's an optional feature. I don't
> think there's much sense in this separation and the use of
> __must_check. Therefore remove pci_try_set_mwi() and remove the
> __must_check attribute from pci_set_mwi().
> I don't expect either function to be used in new code anyway.
You probably want to elaborate here that the feature is specific to
PCI and isn't present on PCIe.
Besides that one comment below.
After addressing, have my
Reviewed-by: Andy Shevchenko
for the files left in this message.
...
> drivers/dma/dw/pci.c | 2 +-
> drivers/dma/hsu/pci.c | 2 +-
> drivers/mfd/intel-lpss-pci.c | 2 +-
> drivers/pci/pci.c | 19 ---
> drivers/tty/serial/8250/8250_lpss.c | 2 +-
> drivers/usb/chipidea/ci_hdrc_pci.c| 2 +-
> drivers/usb/gadget/udc/pch_udc.c | 2 +-
> include/linux/pci.h | 5 ++---
> diff --git a/Documentation/PCI/pci.rst b/Documentation/PCI/pci.rst
> index 814b40f83..120362cc9 100644
> --- a/Documentation/PCI/pci.rst
> +++ b/Documentation/PCI/pci.rst
> @@ -226,10 +226,7 @@ If the PCI device can use the PCI
> Memory-Write-Invalidate transaction,
> call pci_set_mwi(). This enables the PCI_COMMAND bit for Mem-Wr-Inval
> and also ensures that the cache line size register is set correctly.
> Check the return value of pci_set_mwi() as not all architectures
> -or chip-sets may support Memory-Write-Invalidate. Alternatively,
> -if Mem-Wr-Inval would be nice to have but is not required, call
> -pci_try_set_mwi() to have the system do its best effort at enabling
> -Mem-Wr-Inval.
> +or chip-sets may support Memory-Write-Invalidate.
...
> diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
> index 1142aa6f8..1c20b7485 100644
> --- a/drivers/dma/dw/pci.c
> +++ b/drivers/dma/dw/pci.c
> @@ -30,7 +30,7 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *pid)
> }
>
> pci_set_master(pdev);
> - pci_try_set_mwi(pdev);
> + pci_set_mwi(pdev);
>
> ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> if (ret)
> diff --git a/drivers/dma/hsu/pci.c b/drivers/dma/hsu/pci.c
> index 07cc7320a..420dd3706 100644
> --- a/drivers/dma/hsu/pci.c
> +++ b/drivers/dma/hsu/pci.c
> @@ -73,7 +73,7 @@ static int hsu_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> }
>
> pci_set_master(pdev);
> - pci_try_set_mwi(pdev);
> + pci_set_mwi(pdev);
>
> ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> if (ret)
...
> diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
> index 2d7c588ef..a0c3be750 100644
> --- a/drivers/mfd/intel-lpss-pci.c
> +++ b/drivers/mfd/intel-lpss-pci.c
> @@ -39,7 +39,7 @@ static int intel_lpss_pci_probe(struct pci_dev *pdev,
>
> /* Probably it is enough to set this for iDMA capable devices only */
> pci_set_master(pdev);
> - pci_try_set_mwi(pdev);
> + pci_set_mwi(pdev);
>
> ret = intel_lpss_probe(&pdev->dev, info);
> if (ret)
...
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9a5500287..f0ab432d2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4389,25 +4389,6 @@ int pcim_set_mwi(struct pci_dev *dev)
> }
> EXPORT_SYMBOL(pcim_set_mwi);
>
> -/**
> - * pci_try_set_mwi - enables memory-write-invalidate PCI transaction
> - * @dev: the PCI device for which MWI is enabled
> - *
> - * Enables the Memory-Write-Invalidate transaction in %PCI_COMMAND.
> - * Callers are not required to check the return value.
> - *
> - * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
> - */
> -int pci_try_set_mwi(struct pci_dev *dev)
> -{
> -#ifdef PCI_DISABLE_MWI
> - return 0;
> -#else
> - return pci_set_mwi(dev);
> -#endif
This seems still valid case for PowerPC and SH.
> -}
> -EXPORT_SYMBOL(pci_try_set_mwi);
...
> diff --git a/drivers/tty/serial/8250/8250_lpss.c
> b/drivers/tty/serial/8250/8250_lpss.c
> index 4dee8a9e0..8acc1e5c9 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -193,7 +193,7 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss,
> struct uart_port *port)
> if (ret)
> return;
>
> -
Re: [PATCH v2] NFC: nxp-nci: Make firmware GPIO pin optional
On Tue, Dec 1, 2020 at 10:12 AM Schrempf Frieder
wrote:
>
> From: Frieder Schrempf
>
> There are other NXP NCI compatible NFC controllers such as the PN7150
> that use an integrated firmware and therefore do not have a GPIO to
> select firmware downloading mode. To support these kind of chips,
these -> this
OR
kind -> kinds
> let's make the firmware GPIO optional.
FWIW,
Reviewed-by: Andy Shevchenko
> Signed-off-by: Frieder Schrempf
>
> ---
> Changes in v2:
> * Remove unneeded null check for phy->gpiod_fw
> ---
> Documentation/devicetree/bindings/net/nfc/nxp-nci.txt | 2 +-
> drivers/nfc/nxp-nci/i2c.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/nfc/nxp-nci.txt
> b/Documentation/devicetree/bindings/net/nfc/nxp-nci.txt
> index cfaf88998918..cb2385c277d0 100644
> --- a/Documentation/devicetree/bindings/net/nfc/nxp-nci.txt
> +++ b/Documentation/devicetree/bindings/net/nfc/nxp-nci.txt
> @@ -6,11 +6,11 @@ Required properties:
> - reg: address on the bus
> - interrupts: GPIO interrupt to which the chip is connected
> - enable-gpios: Output GPIO pin used for enabling/disabling the chip
> -- firmware-gpios: Output GPIO pin used to enter firmware download mode
>
> Optional SoC Specific Properties:
> - pinctrl-names: Contains only one value - "default".
> - pintctrl-0: Specifies the pin control groups used for this controller.
> +- firmware-gpios: Output GPIO pin used to enter firmware download mode
>
> Example (for ARM-based BeagleBone with NPC100 NFC controller on I2C2):
>
> diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
> index 9f60e4dc5a90..7e451c10985d 100644
> --- a/drivers/nfc/nxp-nci/i2c.c
> +++ b/drivers/nfc/nxp-nci/i2c.c
> @@ -286,7 +286,7 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
> return PTR_ERR(phy->gpiod_en);
> }
>
> - phy->gpiod_fw = devm_gpiod_get(dev, "firmware", GPIOD_OUT_LOW);
> + phy->gpiod_fw = devm_gpiod_get_optional(dev, "firmware",
> GPIOD_OUT_LOW);
> if (IS_ERR(phy->gpiod_fw)) {
> nfc_err(dev, "Failed to get FW gpio\n");
> return PTR_ERR(phy->gpiod_fw);
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] NFC: nxp-nci: Make firmware GPIO pin optional
On Mon, Nov 30, 2020 at 1:06 PM Schrempf Frieder wrote: > > From: Frieder Schrempf > > There are other NXP NCI compatible NFC controllers such as the PN7150 > that use an integrated firmware and therefore do not have a GPIO to > select firmware downloading mode. To support these kind of chips, > let's make the firmware GPIO optional. ... > - gpiod_set_value(phy->gpiod_fw, (mode == NXP_NCI_MODE_FW) ? 1 : 0); > + if (phy->gpiod_fw) > + gpiod_set_value(phy->gpiod_fw, (mode == NXP_NCI_MODE_FW) ? 1 > : 0); This change is not needed. -- With Best Regards, Andy Shevchenko
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 10:39 PM James Bottomley wrote: > On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote: > > On Mon, Nov 23, 2020 at 4:58 PM James Bottomley > > wrote: ... > > But if we do the math, for an author, at even 1 minute per line > > change and assuming nothing can be automated at all, it would take 1 > > month of work. For maintainers, a couple of trivial lines is noise > > compared to many other patches. > > So you think a one line patch should take one minute to produce ... I > really don't think that's grounded in reality. I suppose a one line > patch only takes a minute to merge with b4 if no-one reviews or tests > it, but that's not really desirable. In my practice most of the one line patches were either to fix or to introduce quite interesting issues. 1 minute is 2-3 orders less than usually needed for such patches. That's why I don't like churn produced by people who often even didn't compile their useful contributions. -- With Best Regards, Andy Shevchenko
Re: [PATCH net v2] net: phy: spi_ks8995: Do not overwrite SPI mode flags
On Tue, Nov 10, 2020 at 4:20 PM Sven Van Asbroeck wrote: > > From: Sven Van Asbroeck > > This driver makes sure the underlying SPI bus is set to "mode 0" > by assigning SPI_MODE_0 to spi->mode. Which overwrites all other > SPI mode flags. > > In some circumstances, this can break the underlying SPI bus driver. > For example, if SPI_CS_HIGH is set on the SPI bus, the driver > will clear that flag, which results in a chip-select polarity issue. > > Fix by changing only the SPI_MODE_N bits, i.e. SPI_CPHA and SPI_CPOL. I see that this is a fix for backporing, but maybe you can send a patches on top of this to: 1) introduce #define SPI_MODE_MASK (SPI_CPHA | SPI_CPOL) > + /* use SPI_MODE_0 without changing any other mode flags */ > + spi->mode &= ~(SPI_CPHA | SPI_CPOL); 2) spi->mode &= ~SPI_MODE_MASK; > + spi->mode |= SPI_MODE_0; ? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 0/8] slab: provide and use krealloc_array()
On Tue, Nov 3, 2020 at 12:13 PM Bartosz Golaszewski wrote: > On Tue, Nov 3, 2020 at 5:14 AM Joe Perches wrote: > > On Mon, 2020-11-02 at 16:20 +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski > Yeah so I had this concern for devm_krealloc() and even sent a patch > that extended it to honor __GFP_ZERO before I noticed that regular > krealloc() silently ignores __GFP_ZERO. I'm not sure if this is on > purpose. Maybe we should either make krealloc() honor __GFP_ZERO or > explicitly state in its documentation that it ignores it? And my voice here is to ignore for the same reasons: respect realloc(3) and making common sense with the idea of REallocating (capital letters on purpose). -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 8/8] dma-buf: use krealloc_array()
On Mon, Nov 02, 2020 at 04:20:37PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > Use the helper that checks for overflows internally instead of manually > calculating the size of the new array. ... > + nfences = krealloc_array(fences, i, > + sizeof(*fences), GFP_KERNEL); On 80 position is closing parenthesis, which, I think, makes it okay to put on one line. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/3] mwifiex: add allow_ps_mode module parameter
On Fri, Oct 30, 2020 at 04:58:33PM +0900, Tsuchiya Yuto wrote: > On Wed, 2020-10-28 at 15:04 -0700, Brian Norris wrote: ... > On the other hand, I agree that I don't want to break the existing users. > As you mentioned in the reply to the first patch, I can set the default > value of this parameter depending on the chip id (88W8897) or DMI matching. Since it's a PCIe device you already have ID table where you may add a driver_data with what ever quirks are needed. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default instead
On Thu, Oct 29, 2020 at 8:29 PM Brian Norris wrote: > On Wed, Oct 28, 2020 at 7:04 PM Tsuchiya Yuto wrote: ... > For the record, Chrome OS supports plenty of mwifiex systems with 8897 > (SDIO only) and 8997 (PCIe), with PS enabled, and you're hurting > those. Your problem sounds to be exclusively a problem with the PCIe > 8897 firmware. And this feeling (that it's a FW issue) what I have. But the problem here, that Marvell didn't fix and probably won't fix their FW... Just wondering if Google (and MS in their turn) use different firmwares to what we have available in Linux. -- With Best Regards, Andy Shevchenko
[PATCH v3] net: phy: leds: Deduplicate link LED trigger registration
Refactor phy_led_trigger_register() and deduplicate its functionality
when registering LED trigger for link.
Signed-off-by: Andy Shevchenko
Reviewed-by: Andrew Lunn
---
v3: rebased on top of v5.10-rc1
drivers/net/phy/phy_led_triggers.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/phy/phy_led_triggers.c
b/drivers/net/phy/phy_led_triggers.c
index 59a94e07e7c5..08a3e9ea4102 100644
--- a/drivers/net/phy/phy_led_triggers.c
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -66,11 +66,11 @@ static void phy_led_trigger_format_name(struct phy_device
*phy, char *buf,
static int phy_led_trigger_register(struct phy_device *phy,
struct phy_led_trigger *plt,
- unsigned int speed)
+ unsigned int speed,
+ const char *suffix)
{
plt->speed = speed;
- phy_led_trigger_format_name(phy, plt->name, sizeof(plt->name),
- phy_speed_to_str(speed));
+ phy_led_trigger_format_name(phy, plt->name, sizeof(plt->name), suffix);
plt->trigger.name = plt->name;
return led_trigger_register(&plt->trigger);
@@ -99,12 +99,7 @@ int phy_led_triggers_register(struct phy_device *phy)
goto out_clear;
}
- phy_led_trigger_format_name(phy, phy->led_link_trigger->name,
- sizeof(phy->led_link_trigger->name),
- "link");
- phy->led_link_trigger->trigger.name = phy->led_link_trigger->name;
-
- err = led_trigger_register(&phy->led_link_trigger->trigger);
+ err = phy_led_trigger_register(phy, phy->led_link_trigger, 0, "link");
if (err)
goto out_free_link;
@@ -119,7 +114,7 @@ int phy_led_triggers_register(struct phy_device *phy)
for (i = 0; i < phy->phy_num_led_triggers; i++) {
err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
- speeds[i]);
+ speeds[i],
phy_speed_to_str(speeds[i]));
if (err)
goto out_unreg;
}
--
2.28.0
Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Wed, Sep 30, 2020 at 7:06 PM Calvin Johnson
wrote:
>
> Modify dpaa2_mac_connect() to support ACPI along with DT.
> Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
> DT or ACPI.
>
> Replace of_get_phy_mode with fwnode_get_phy_mode to get
> phy-mode for a dpmac_node.
>
> Use helper function phylink_fwnode_phy_connect() to find phy_dev and
> connect to mac->phylink.
...
> #include "dpaa2-eth.h"
> #include "dpaa2-mac.h"
> +#include
Please, put generic headers first.
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct fwnode_handle *dpmacs, *dpmac = NULL;
> + unsigned long long adr;
> + acpi_status status;
> int err;
> + u32 id;
>
> - dpmacs = of_find_node_by_name(NULL, "dpmacs");
> - if (!dpmacs)
> - return NULL;
> + if (is_of_node(dev->parent->fwnode)) {
> + dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> + if (!dpmacs)
> + return NULL;
> +
> + while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> + err = fwnode_property_read_u32(dpmac, "reg", &id);
> + if (err)
> + continue;
> + if (id == dpmac_id)
> + return dpmac;
> + }
>
> + } else if (is_acpi_node(dev->parent->fwnode)) {
> + device_for_each_child_node(dev->parent, dpmac) {
> + status =
> acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> + "_ADR", NULL, &adr);
> + if (ACPI_FAILURE(status)) {
> + pr_debug("_ADR returned %d on %s\n",
> +status, (char *)buffer.pointer);
> + continue;
> + } else {
> + id = (u32)adr;
> + if (id == dpmac_id)
> + return dpmac;
> + }
> + }
Can you rather implement generic one which will be
int fwnode_get_child_id(struct fwnode_handle *fwnode, u64 *id);
and put the logic of retrieving 'reg' or _ADR? Also, for the latter we
have a special macro
METHOD_NAME__ADR.
See [1] as well. Same idea I have shared already.
[1]:
https://lore.kernel.org/linux-iio/20200824054347.3805-1-william.s...@advantech.com.tw/T/#m5f61921fa67a5b40522b7f7b17216e0d204647be
...
> - of_node_put(dpmac_node);
> + if (is_of_node(dpmac_node))
> + of_node_put(to_of_node(dpmac_node));
I'm not sure why you can't use fwnode_handle_put()?
> + if (is_of_node(dpmac_node))
> + of_node_put(to_of_node(dpmac_node));
Ditto.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v7 1/6] Documentation: ACPI: DSD: Document MDIO PHY
On Tue, Sep 29, 2020 at 5:32 PM Andrew Lunn wrote: > > On Tue, Sep 29, 2020 at 04:55:40PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn wrote: > > > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > > > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > > > > ... > > > > > Newbie ACPI question: Does ACPI even support big endian CPUs, given > > > its x86 origins? > > > > I understand the newbie part, but can you elaborate what did you mean > > under 'support'? > > To me it sounds like 'network stack was developed for BE CPUs, does it > > support LE ones?' > > Does ACPI define the endianness of its tables? Is it written in the > standard that they should be little endian? 5.2: "All numeric values in ACPI-defined tables, blocks, and structures are always encoded in little endian format. Signature values are stored as fixed-length strings." > Does Tianocore, or any > other implementations, have the needed le32_to_cpu() calls so that > they can boot on a big endian CPU? Not of my knowledge. > Does it have a standardized way of > saying a device is big endian, swap words around if appropriate when > doing IO? I guess this is not applicable to ACPI. Does Linux have a standardized way? So, what did you mean under doing I/O? I mean in which context? > Is it feasible to boot an ARM system big endian? Not an ARM guy. > Can i boot the same > system little endian? The CPU should be able to do it, but are the > needed things in the ACPI specification and implementation to allow > it? -- With Best Regards, Andy Shevchenko
Re: [net-next PATCH v7 1/6] Documentation: ACPI: DSD: Document MDIO PHY
On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn wrote: > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: ... > Newbie ACPI question: Does ACPI even support big endian CPUs, given > its x86 origins? I understand the newbie part, but can you elaborate what did you mean under 'support'? To me it sounds like 'network stack was developed for BE CPUs, does it support LE ones?' -- With Best Regards, Andy Shevchenko
Re: [PATCH net 1/1] net: stmmac: Fix clock handling on remove path
On Fri, Sep 25, 2020 at 12:54 PM Wong Vee Khee wrote:
>
> While unloading the dwmac-intel driver, clk_disable_unprepare() is
> being called twice in stmmac_dvr_remove() and
> intel_eth_pci_remove(). This causes kernel panic on the second call.
>
> Removing the second call of clk_disable_unprepare() in
> intel_eth_pci_remove().
Thanks! I'm not sure how I missed this...
Reviewed-by: Andy Shevchenko
> Fixes: 09f012e64e4b ("stmmac: intel: Fix clock handling on error and remove
> paths")
> Cc: Andy Shevchenko
> Reviewed-by: Voon Weifeng
> Signed-off-by: Wong Vee Khee
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 2ac9dfb3462c..9e6d60e75f85 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -653,7 +653,6 @@ static void intel_eth_pci_remove(struct pci_dev *pdev)
>
> pci_free_irq_vectors(pdev);
>
> - clk_disable_unprepare(priv->plat->stmmac_clk);
> clk_unregister_fixed_rate(priv->plat->stmmac_clk);
>
> pcim_iounmap_regions(pdev, BIT(0));
> --
> 2.17.0
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next v2] net: phy: leds: Deduplicate link LED trigger registration
On Wed, Aug 26, 2020 at 06:22:23PM +0300, Andy Shevchenko wrote:
> Refactor phy_led_trigger_register() and deduplicate its functionality
> when registering LED trigger for link.
Is it good enough now?
> Signed-off-by: Andy Shevchenko
> Reviewed-by: Andrew Lunn
> ---
> v2: fixed build error (lkp, David)
> drivers/net/phy/phy_led_triggers.c | 15 +--
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/phy/phy_led_triggers.c
> b/drivers/net/phy/phy_led_triggers.c
> index 59a94e07e7c5..08a3e9ea4102 100644
> --- a/drivers/net/phy/phy_led_triggers.c
> +++ b/drivers/net/phy/phy_led_triggers.c
> @@ -66,11 +66,11 @@ static void phy_led_trigger_format_name(struct phy_device
> *phy, char *buf,
>
> static int phy_led_trigger_register(struct phy_device *phy,
> struct phy_led_trigger *plt,
> - unsigned int speed)
> + unsigned int speed,
> + const char *suffix)
> {
> plt->speed = speed;
> - phy_led_trigger_format_name(phy, plt->name, sizeof(plt->name),
> - phy_speed_to_str(speed));
> + phy_led_trigger_format_name(phy, plt->name, sizeof(plt->name), suffix);
> plt->trigger.name = plt->name;
>
> return led_trigger_register(&plt->trigger);
> @@ -99,12 +99,7 @@ int phy_led_triggers_register(struct phy_device *phy)
> goto out_clear;
> }
>
> - phy_led_trigger_format_name(phy, phy->led_link_trigger->name,
> - sizeof(phy->led_link_trigger->name),
> - "link");
> - phy->led_link_trigger->trigger.name = phy->led_link_trigger->name;
> -
> - err = led_trigger_register(&phy->led_link_trigger->trigger);
> + err = phy_led_trigger_register(phy, phy->led_link_trigger, 0, "link");
> if (err)
> goto out_free_link;
>
> @@ -119,7 +114,7 @@ int phy_led_triggers_register(struct phy_device *phy)
>
> for (i = 0; i < phy->phy_num_led_triggers; i++) {
> err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
> - speeds[i]);
> +speeds[i],
> phy_speed_to_str(speeds[i]));
> if (err)
> goto out_unreg;
> }
> --
> 2.28.0
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Thu, Sep 10, 2020 at 11:25 AM Vadym Kochan wrote:
> On Fri, Sep 04, 2020 at 10:12:07PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan
> > wrote:
> > > + .param = {.admin_state = admin_state}
> >
> > + white spaces? Whatever you choose, just be consistent among all
> > similar definitions.
> >
>
> Can I use following format for one-liner embedded struct ?
Of course. It's just a style matter.
My point that you need to be consistent over all similar cases in the code.
> .param = {
> .admin_state = admin_state,
> }
> > ...
> >
>
> I think it looks better when all of the members filled looks similar
> (even if it requires 2 additional lines) instead of having:
>
> .member = { E } ?
I like the former one (as you do), but in some cases when you have one
member or an array it's convenient to have them one one line.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Wed, Sep 9, 2020 at 5:00 PM Vadym Kochan wrote:
> On Tue, Sep 08, 2020 at 12:38:04PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 8, 2020 at 11:35 AM Vadym Kochan
> > wrote:
> > > On Fri, Sep 04, 2020 at 10:12:07PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan
> > > > wrote:
...
> > > > > + if (b == 0)
> > > > > + continue;
> > > > > +
> > > > > + prestera_sdma_rx_desc_set_next(sdma,
> > > > > + ring->bufs[b -
> > > > > 1].desc,
> > > > > + buf->desc_dma);
> > > > > +
> > > > > + if (b == PRESTERA_SDMA_RX_DESC_PER_Q - 1)
> > > > > + prestera_sdma_rx_desc_set_next(sdma,
> > > > > buf->desc,
> > > > > +
> > > > > head->desc_dma);
> > > >
> > > > I guess knowing what the allowed range of bnum the above can be
> > > > optimized.
> > > >
> > > You mean to replace PRESTERA_SDMA_RX_DESC_PER_Q by bnum ?
> >
> > I don't know what you meant in above. It might be a bug, it might be
> > that bnum is redundant and this definition may be used everywhere...
> > But I believe there is room for improvement when I see pattern like
> >
> > for (i < X) {
> > ...
> > if (i == 0) {
> > ...
> > } else if (i == X - 1) {
> > ...
> > }
> > }
> >
> > Either it can be while-loop (or do-while) with better semantics for
> > the first and last item to handle or something else.
> > Example from another review [1] in case you wonder how changes can be
> > made. Just think about it.
> >
> > [1]: https://www.spinics.net/lists/linux-pci/msg60826.html (before)
> > https://www.spinics.net/lists/linux-pci/msg62043.html (after)
> >
>
> I came up with the following approach which works:
>
> -->8
> tail = &ring->bufs[bnum - 1];
> head = &ring->bufs[0];
> next = head;
> prev = next;
> ring->next_rx = 0;
>
> do {
> err = prestera_sdma_buf_init(sdma, next);
> if (err)
> return err;
>
> err = prestera_sdma_rx_skb_alloc(sdma, next);
> if (err)
> return err;
>
> prestera_sdma_rx_desc_init(sdma, next->desc, next->buf_dma);
>
> prestera_sdma_rx_desc_set_next(sdma, prev->desc, next->desc_dma);
>
> prev = next;
> next++;
> } while (prev != tail);
>
> /* make a circular list */
> prestera_sdma_rx_desc_set_next(sdma, tail->desc, head->desc_dma);
> --8<
>
> Now it looks more list-oriented cyclic logic.
And much better, thanks!
Note, you have two places in the code with something similar (I mean
loop handling), perhaps you may improve both.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Tue, Sep 8, 2020 at 11:35 AM Vadym Kochan wrote:
> On Fri, Sep 04, 2020 at 10:12:07PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan
> > wrote:
...
> > > + words[3] |= FIELD_PREP(PRESTERA_W3_HW_DEV_NUM, (dsa->hw_dev_num
> > > >> 5));
> >
> > Ditto.
> >
> I am not sure 5 needs to be defined as macro as it just moves
> hw_dev_num's higher bits into the last word.
And why 5? I want 6, for example!
...
> > > + err = prestera_switch_init(sw);
> > > + if (err) {
> > > + kfree(sw);
> >
> > > + return err;
> > > + }
> > > +
> > > + return 0;
> >
> > return err;
> >
> why not keep 'return 0' as indication of success point ?
Simple longer, but I'm not insisting. Your choice.
...
> > > + if (b == 0)
> > > + continue;
> > > +
> > > + prestera_sdma_rx_desc_set_next(sdma,
> > > + ring->bufs[b -
> > > 1].desc,
> > > + buf->desc_dma);
> > > +
> > > + if (b == PRESTERA_SDMA_RX_DESC_PER_Q - 1)
> > > + prestera_sdma_rx_desc_set_next(sdma,
> > > buf->desc,
> > > +
> > > head->desc_dma);
> >
> > I guess knowing what the allowed range of bnum the above can be optimized.
> >
> You mean to replace PRESTERA_SDMA_RX_DESC_PER_Q by bnum ?
I don't know what you meant in above. It might be a bug, it might be
that bnum is redundant and this definition may be used everywhere...
But I believe there is room for improvement when I see pattern like
for (i < X) {
...
if (i == 0) {
...
} else if (i == X - 1) {
...
}
}
Either it can be while-loop (or do-while) with better semantics for
the first and last item to handle or something else.
Example from another review [1] in case you wonder how changes can be
made. Just think about it.
[1]: https://www.spinics.net/lists/linux-pci/msg60826.html (before)
https://www.spinics.net/lists/linux-pci/msg62043.html (after)
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Mon, Sep 7, 2020 at 10:30 AM Vadym Kochan wrote: > On Fri, Sep 04, 2020 at 10:12:07PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan > > wrote: I'm assuming that you agree on all non-commented. ... > > > +#define prestera_dev(sw) ((sw)->dev->dev) > > > > The point of this is...? (What I see it's 2 characters longer) > > > > ... > It is not about the length but rather about easier semantics, so > the prestera_dev() is more easier to remember than sw->dev->dev. It actually makes it opposite, now I have to go somewhere in the file, quite far from the place where it is used, and check what it is. Then I return to the code I'm reading and after a few more lines of code I will forget what that macro means. ... > > > + /* firmware requires that port's mac address consist of the first > > > +* 5 bytes of base mac address > > > +*/ > > > > > > > + memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1); > > > > Can't you call above helper for that? > > Not sure if I got this right, but I assume that may be just use > ether_addr_copy() and then just perform the below assignment on the > last byte ? No, I mean the function where you have the same comment and something else. I'm wondering if you may call it from here. Or refactor code to make it possible to call from here. -- With Best Regards, Andy Shevchenko
Re: [PATCH net-next v7 5/6] net: marvell: prestera: Add Switchdev driver implementation
On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan wrote: > > The following features are supported: > > - VLAN-aware bridge offloading > - VLAN-unaware bridge offloading > - FDB offloading (learning, ageing) > - Switchport configuration > > Currently there are some limitations like: > > - Only 1 VLAN-aware bridge instance supported > - FDB ageing timeout parameter is set globally per device Similar comments as per previous patches. > + struct list_head vlans_list; How this container is being protected against races? -- With Best Regards, Andy Shevchenko
Re: [PATCH net-next v7 4/6] net: marvell: prestera: Add ethtool interface support
On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan wrote:
>
> The ethtool API provides support for the configuration of the following
> features: speed and duplex, auto-negotiation, MDI-x, forward error
> correction, port media type. The API also provides information about the
> port status, hardware and software statistic. The following limitation
> exists:
>
> - port media type should be configured before speed setting
> - ethtool -m option is not supported
> - ethtool -p option is not supported
> - ethtool -r option is supported for RJ45 port only
> - the following combination of parameters is not supported:
>
> ethtool -s sw1pX port XX autoneg on
>
> - forward error correction feature is supported only on SFP ports, 10G
> speed
>
> - auto-negotiation and MDI-x features are not supported on
> Copper-to-Fiber SFP module
...
> +#include
> +#include
> +#include
Sorted?
...
> + if (new_mode < PRESTERA_LINK_MODE_MAX)
> + err = prestera_hw_port_link_mode_set(port, new_mode);
> + else
> + err = -EINVAL;
> +
> + if (!err) {
> + port->caps.type = type;
> + port->autoneg = false;
> + }
> +
> + return err;
Traditional pattern?
if (err)
return err;
...
return 0;
...
> + ecmd->base.speed = !err ? speed : SPEED_UNKNOWN;
Why not err : SPEED_... : speed; ?
...
> +static int
> +prestera_ethtool_get_link_ksettings(struct net_device *dev,
One line?
> + struct ethtool_link_ksettings *ecmd)
...
> + err = prestera_hw_port_link_mode_get(port, &curr_mode);
> + if (err || curr_mode >= PRESTERA_LINK_MODE_MAX)
> + return -EINVAL;
Why shadowing error code?
...
> +/*
> + * Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved.
> + *
> + */
One line.
...
> +#include
Is this being used?
> +#include
> +
> +extern const struct ethtool_ops prestera_ethtool_ops;
...
> +enum {
> + PRESTERA_FC_NONE,
> + PRESTERA_FC_SYMMETRIC,
> + PRESTERA_FC_ASYMMETRIC,
> + PRESTERA_FC_SYMM_ASYMM
Comma?
> +};
...
> + struct prestera_msg_port_attr_req req = {
> + .attr = PRESTERA_CMD_PORT_ATTR_REMOTE_CAPABILITY,
> + .port = port->hw_id,
> + .dev = port->dev_id
Comma?
> + };
...
> + err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_GET,
> + &req.cmd, sizeof(req), &resp.ret,
> sizeof(resp));
> + if (err)
> + return err;
> +
> + *link_mode_bitmap = resp.param.cap.link_mode;
> +
> + return err;
return 0;
I think I have talked that any of the given comments applies to *all*
similar code pieces!
This file is full of repetitions of the above.
...
> +static u8 prestera_hw_mdix_to_eth(u8 mode)
> +{
> + switch (mode) {
> + case PRESTERA_PORT_TP_MDI:
> + return ETH_TP_MDI;
> + case PRESTERA_PORT_TP_MDIX:
> + return ETH_TP_MDI_X;
> + case PRESTERA_PORT_TP_AUTO:
> + return ETH_TP_MDI_AUTO;
> + }
> +
> + return ETH_TP_MDI_INVALID;
default.
Also I have a deja vu about such.
> +}
> +
> +static u8 prestera_hw_mdix_from_eth(u8 mode)
> +{
> + switch (mode) {
> + case ETH_TP_MDI:
> + return PRESTERA_PORT_TP_MDI;
> + case ETH_TP_MDI_X:
> + return PRESTERA_PORT_TP_MDIX;
> + case ETH_TP_MDI_AUTO:
> + return PRESTERA_PORT_TP_AUTO;
> + }
> +
> + return PRESTERA_PORT_TP_NA;
> +}
Ditto.
...
> +enum {
> + PRESTERA_PORT_DUPLEX_HALF,
> + PRESTERA_PORT_DUPLEX_FULL
Comma ?
> +};
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next v7 2/6] net: marvell: prestera: Add PCI interface support
On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan wrote:
>
> Add PCI interface driver for Prestera Switch ASICs family devices, which
> provides:
>
> - Firmware loading mechanism
> - Requests & events handling to/from the firmware
> - Access to the firmware on the bus level
>
> The firmware has to be loaded each time the device is reset. The driver
> is loading it from:
>
> /lib/firmware/mrvl/prestera/mvsw_prestera_fw-v{MAJOR}.{MINOR}.img
>
> The full firmware image version is located within the internal header
> and consists of 3 numbers - MAJOR.MINOR.PATCH. Additionally, driver has
> hard-coded minimum supported firmware version which it can work with:
>
> MAJOR - reflects the support on ABI level between driver and loaded
> firmware, this number should be the same for driver and loaded
> firmware.
>
> MINOR - this is the minimum supported version between driver and the
> firmware.
>
> PATCH - indicates only fixes, firmware ABI is not changed.
>
> Firmware image file name contains only MAJOR and MINOR numbers to make
> driver be compatible with any PATCH version.
...
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Perhaps sorted?
...
> +enum prestera_pci_bar_t {
> + PRESTERA_PCI_BAR_FW = 2,
> + PRESTERA_PCI_BAR_PP = 4
Comma?
> +};
...
> + return readl_poll_timeout(addr, rd_idx,
> +CIRC_SPACE(wr_idx, rd_idx, buf_len) >= len,
> +1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> + return 0;
dead code.
...
> + if (err) {
> + dev_err(fw->dev.dev, "Timeout to load FW img [state=%d]",
> + prestera_ldr_read(fw, PRESTERA_LDR_STATUS_REG));
> + return err;
> + }
> +
> + return 0;
return err;
> +}
...
> + status = prestera_ldr_read(fw, PRESTERA_LDR_STATUS_REG);
> + if (status != PRESTERA_LDR_STATUS_START_FW) {
Point of this check?
> + switch (status) {
> + case PRESTERA_LDR_STATUS_INVALID_IMG:
> + dev_err(fw->dev.dev, "FW img has bad CRC\n");
> + return -EINVAL;
> + case PRESTERA_LDR_STATUS_NOMEM:
> + dev_err(fw->dev.dev, "Loader has no enough mem\n");
> + return -ENOMEM;
> + default:
> + break;
> + }
> + }
> +
> + return 0;
...
> + err = prestera_ldr_wait_reg32(fw, PRESTERA_LDR_READY_REG,
> + PRESTERA_LDR_READY_MAGIC, 5 * 1000);
1000? MSEC_PER_SEC?
> + if (err) {
> + dev_err(fw->dev.dev, "waiting for FW loader is timed out");
> + return err;
> + }
...
> + if (!IS_ALIGNED(f->size, 4)) {
> + dev_err(fw->dev.dev, "FW image file is not aligned");
> + release_firmware(f);
> + return -EINVAL;
err = -EINVAL;
goto out_release;
?
> + }
Is it really fatal?
> +
> + err = prestera_fw_hdr_parse(fw, f);
> + if (err) {
> + dev_err(fw->dev.dev, "FW image header is invalid\n");
> + release_firmware(f);
> + return err;
goto out_release; ?
> + }
> +
> + prestera_ldr_write(fw, PRESTERA_LDR_IMG_SIZE_REG, f->size - hlen);
> + prestera_ldr_write(fw, PRESTERA_LDR_CTL_REG,
> PRESTERA_LDR_CTL_DL_START);
> +
> + dev_info(fw->dev.dev, "Loading %s ...", fw_path);
> +
> + err = prestera_ldr_fw_send(fw, f->data + hlen, f->size - hlen);
out_release: ?
> + release_firmware(f);
> + return err;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
+ (PRESTERA_SDMA_RX_DESC_OWNER((desc)) == PRESTERA_SDMA_RX_DESC_CPU_OWN)
Double (()) make no sense here.
...
> +static void prestera_sdma_rx_desc_set_len(struct prestera_sdma_desc *desc,
> + size_t val)
> +{
> + u32 word = le32_to_cpu(desc->word2);
> +
> + word = (word & ~GENMASK(15, 0)) | val;
> + desc->word2 = cpu_to_le32(word);
> +}
Why not simple le32_replace_bits() ?
...
> + port = prestera_port_find_by_hwid(sdma->sw, dev_id, hw_port);
> + if (unlikely(!port)) {
> + pr_warn_ratelimited("received pkt for non-existent port(%u,
> %u)\n",
> + dev_id, hw_port);
What's wrong with dev_warn_ratelimited() ?
> + return -EEXIST;
> + }
...
> + for (b = 0; b < bnum; b++) {
> + struct prestera_sdma_buf *buf = &ring->bufs[b];
> +
> + err = prestera_sdma_buf_init(sdma, buf);
> + if (err)
> + return err;
> +
> + err = prestera_sdma_rx_skb_alloc(sdma, buf);
> + if (err)
No need to uninit previously init buffers?
No need to dealloc previously allocated stuff?
> + return err;
...
> + if (b == 0)
> + continue;
> +
> + prestera_sdma_rx_desc_set_next(sdma,
> + ring->bufs[b - 1].desc,
> + buf->desc_dma);
> +
> + if (b == PRESTERA_SDMA_RX_DESC_PER_Q - 1)
> + prestera_sdma_rx_desc_set_next(sdma,
> buf->desc,
> +
> head->desc_dma);
I guess knowing what the allowed range of bnum the above can be optimized.
> + }
...
> + u32 word = le32_to_cpu(desc->word2);
> +
> + word = (word & ~GENMASK(30, 16)) | ((len + ETH_FCS_LEN) << 16);
> +
> + desc->word2 = cpu_to_le32(word);
le32_replace_bits()
...
> +static void prestera_sdma_tx_desc_xmit(struct prestera_sdma_desc *desc)
> +{
> + u32 word = le32_to_cpu(desc->word1);
> +
> + word |= PRESTERA_SDMA_TX_DESC_DMA_OWN << 31;
> +
> + /* make sure everything is written before enable xmit */
> + wmb();
> +
> + desc->word1 = cpu_to_le32(word);
Seems to me it's also safe to use le32_replace_bits(), but I'm not
sure if desc->word1 can be changed after barrier by somebody else.
> +}
...
> + for (b = 0; b < bnum; b++) {
> + struct prestera_sdma_buf *buf = &tx_ring->bufs[b];
> +
> + err = prestera_sdma_buf_init(sdma, buf);
> + if (err)
> + return err;
> +
> + prestera_sdma_tx_desc_init(sdma, buf->desc);
> +
> + buf->is_used = false;
> +
> + if (b == 0)
> + continue;
> +
> + prestera_sdma_tx_desc_set_next(sdma, tx_ring->bufs[b -
> 1].desc,
> + buf->desc_dma);
> +
> + if (b == PRESTERA_SDMA_TX_DESC_PER_Q - 1)
> + prestera_sdma_tx_desc_set_next(sdma, buf->desc,
> + head->desc_dma);
> + }
Similar comments as per above similar for-loop.
...
> +static int prestera_sdma_tx_wait(struct prestera_sdma *sdma,
> +struct prestera_tx_ring *tx_ring)
> +{
> + int tx_retry_num = PRESTERA_SDMA_WAIT_MUL * tx_ring->max_burst;
> +
> + do {
> + if (prestera_sdma_is_ready(sdma))
> + return 0;
> +
> + udelay(1);
> + } while (--tx_retry_num);
> +
> + return -EBUSY;
> +}
iopoll.h
readx_poll_timeout().
...
> +/*
> + * Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved.
> + *
> + */
One line.
...
> +#include "prestera.h"
I don't see users of the above header here. You may use declarations instead.
> +int prestera_rxtx_switch_init(struct prestera_switch *sw);
> +void prestera_rxtx_switch_fini(struct prestera_switch *sw);
> +
> +int prestera_rxtx_port_init(struct prestera_port *port);
> +
> +netdev_tx_t prestera_rxtx_xmit(struct prestera_port *port, struct sk_buff
> *skb);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH net v6 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Thu, Sep 3, 2020 at 6:23 PM Willem de Bruijn
wrote:
> On Wed, Sep 2, 2020 at 5:37 PM Vadym Kochan wrote:
...
> > +static int prestera_is_valid_mac_addr(struct prestera_port *port, u8 *addr)
> > +{
> > + if (!is_valid_ether_addr(addr))
> > + return -EADDRNOTAVAIL;
> > +
> > + if (memcmp(port->sw->base_mac, addr, ETH_ALEN - 1))
>
> Why ETH_ALEN - 1?
We even have a lot of helpers specifically for ethernet MACs.
Starting from [1] till almost the end of the file. Here [2] can be
used (or its unaligned counterpart).
[1]:
https://elixir.bootlin.com/linux/latest/source/include/linux/etherdevice.h#L67
[2]:
https://elixir.bootlin.com/linux/latest/source/include/linux/etherdevice.h#L67
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > + memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
>
> Is addr_len ever not ETH_ALEN for this device?
And if it is ETH_ALEN, here is [3].
[3]:
https://elixir.bootlin.com/linux/latest/source/include/linux/etherdevice.h#L287
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] net: openvswitch: pass NULL for unused parameters
On Sun, Aug 30, 2020 at 6:17 PM wrote:
>
> From: Tom Rix
>
> clang static analysis flags these problems
>
> flow_table.c:713:2: warning: The expression is an uninitialized
> value. The computed value will also be garbage
> (*n_mask_hit)++;
> ^~~
> flow_table.c:748:5: warning: The expression is an uninitialized
> value. The computed value will also be garbage
> (*n_cache_hit)++;
> ^~~~
>
> These are not problems because neither pararmeter is used
parameter
> by the calling function.
>
> Looking at all of the calling functions, there are many
> cases where the results are unused. Passing unused
> parameters is a waste.
>
> To avoid passing unused parameters, rework the
> masked_flow_lookup() and flow_lookup() routines to check
> for NULL parameters and change the unused parameters to NULL.
>
> Signed-off-by: Tom Rix
> ---
> net/openvswitch/flow_table.c | 16 +++-
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> index e2235849a57e..18e7fa3aa67e 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -710,7 +710,8 @@ static struct sw_flow *masked_flow_lookup(struct
> table_instance *ti,
> ovs_flow_mask_key(&masked_key, unmasked, false, mask);
> hash = flow_hash(&masked_key, &mask->range);
> head = find_bucket(ti, hash);
> - (*n_mask_hit)++;
> + if (n_mask_hit)
> + (*n_mask_hit)++;
>
> hlist_for_each_entry_rcu(flow, head, flow_table.node[ti->node_ver],
> lockdep_ovsl_is_held()) {
> @@ -745,7 +746,8 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
> u64_stats_update_begin(&ma->syncp);
> usage_counters[*index]++;
> u64_stats_update_end(&ma->syncp);
> - (*n_cache_hit)++;
> + if (n_cache_hit)
> + (*n_cache_hit)++;
> return flow;
> }
> }
> @@ -798,9 +800,8 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct
> flow_table *tbl,
> *n_cache_hit = 0;
> if (unlikely(!skb_hash || mc->cache_size == 0)) {
> u32 mask_index = 0;
> - u32 cache = 0;
>
> - return flow_lookup(tbl, ti, ma, key, n_mask_hit, &cache,
> + return flow_lookup(tbl, ti, ma, key, n_mask_hit, NULL,
>&mask_index);
Can it be done for mask_index as well?
> }
>
> @@ -849,11 +850,9 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table
> *tbl,
> {
> struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
> struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
> - u32 __always_unused n_mask_hit;
> - u32 __always_unused n_cache_hit;
> u32 index = 0;
>
> - return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &n_cache_hit,
> &index);
> + return flow_lookup(tbl, ti, ma, key, NULL, NULL, &index);
Ditto.
> }
>
> struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> @@ -865,7 +864,6 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct
> flow_table *tbl,
> /* Always called under ovs-mutex. */
> for (i = 0; i < ma->max; i++) {
> struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
> - u32 __always_unused n_mask_hit;
> struct sw_flow_mask *mask;
> struct sw_flow *flow;
>
> @@ -873,7 +871,7 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct
> flow_table *tbl,
> if (!mask)
> continue;
>
> - flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
> + flow = masked_flow_lookup(ti, match->key, mask, NULL);
> if (flow && ovs_identifier_is_key(&flow->id) &&
> ovs_flow_cmp_unmasked_key(flow, match)) {
> return flow;
> --
> 2.18.1
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1] net: phy: leds: Deduplicate link LED trigger registration
On Tue, Aug 25, 2020 at 07:40:15AM -0700, David Miller wrote: > From: Andy Shevchenko > Date: Mon, 24 Aug 2020 20:09:04 +0300 > > > Refactor phy_led_trigger_register() and deduplicate its functionality > > when registering LED trigger for link. > > > > Signed-off-by: Andy Shevchenko My bad... Missed LED_CLASS and saw even something has been built in drivers/net/ but apparently wasn't enough to get this one compiled. v2 will fix that. Thanks! > This doesn't compile: > > CC [M] drivers/net/phy/phy_led_triggers.o > drivers/net/phy/phy_led_triggers.c: In function ‘phy_led_triggers_register’: > drivers/net/phy/phy_led_triggers.c:102:38: error: passing argument 2 of > ‘phy_led_trigger_register’ from incompatible pointer type > [-Werror=incompatible-pointer-types] > 102 | err = phy_led_trigger_register(phy, &phy->led_link_trigger, 0, > "link"); > | ^~ > | | > | struct phy_led_trigger ** > drivers/net/phy/phy_led_triggers.c:68:33: note: expected ‘struct > phy_led_trigger *’ but argument is of type ‘struct phy_led_trigger **’ >68 | struct phy_led_trigger *plt, > | ^~~ > -- With Best Regards, Andy Shevchenko
Re: [PATCH v1] net: phy: leds: Deduplicate link LED trigger registration
On Mon, Aug 24, 2020 at 07:45:58PM +0200, Andrew Lunn wrote: > On Mon, Aug 24, 2020 at 08:09:04PM +0300, Andy Shevchenko wrote: > > Refactor phy_led_trigger_register() and deduplicate its functionality > > when registering LED trigger for link. > > > > Signed-off-by: Andy Shevchenko > > Reviewed-by: Andrew Lunn Thanks! > Hi Andy > > Please take a look at > https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html Anything particular? I suspect you want me to put net-next prefix... -- With Best Regards, Andy Shevchenko
[PATCH v1] net: phy: leds: Deduplicate link LED trigger registration
Refactor phy_led_trigger_register() and deduplicate its functionality
when registering LED trigger for link.
Signed-off-by: Andy Shevchenko
---
drivers/net/phy/phy_led_triggers.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/phy/phy_led_triggers.c
b/drivers/net/phy/phy_led_triggers.c
index 59a94e07e7c5..9a92e471400e 100644
--- a/drivers/net/phy/phy_led_triggers.c
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -66,11 +66,11 @@ static void phy_led_trigger_format_name(struct phy_device
*phy, char *buf,
static int phy_led_trigger_register(struct phy_device *phy,
struct phy_led_trigger *plt,
- unsigned int speed)
+ unsigned int speed,
+ const char *suffix)
{
plt->speed = speed;
- phy_led_trigger_format_name(phy, plt->name, sizeof(plt->name),
- phy_speed_to_str(speed));
+ phy_led_trigger_format_name(phy, plt->name, sizeof(plt->name), suffix);
plt->trigger.name = plt->name;
return led_trigger_register(&plt->trigger);
@@ -99,12 +99,7 @@ int phy_led_triggers_register(struct phy_device *phy)
goto out_clear;
}
- phy_led_trigger_format_name(phy, phy->led_link_trigger->name,
- sizeof(phy->led_link_trigger->name),
- "link");
- phy->led_link_trigger->trigger.name = phy->led_link_trigger->name;
-
- err = led_trigger_register(&phy->led_link_trigger->trigger);
+ err = phy_led_trigger_register(phy, &phy->led_link_trigger, 0, "link");
if (err)
goto out_free_link;
@@ -119,7 +114,7 @@ int phy_led_triggers_register(struct phy_device *phy)
for (i = 0; i < phy->phy_num_led_triggers; i++) {
err = phy_led_trigger_register(phy, &phy->phy_led_triggers[i],
- speeds[i]);
+ speeds[i],
phy_speed_to_str(speeds[i]));
if (err)
goto out_unreg;
}
--
2.28.0
