Re: [PATCH v2] drivers: net: xgene: Remove acpi_has_method() calls
On Tue, Jul 23, 2019 at 02:07:39PM -0700, David Miller wrote: > From: David Miller > Date: Tue, 23 Jul 2019 14:06:46 -0700 (PDT) > > > From: Kelsey Skunberg > > Date: Tue, 23 Jul 2019 12:58:11 -0600 > > > >> acpi_evaluate_object will already return an error if the needed method > >> does not exist. Remove unnecessary acpi_has_method() calls and check the > >> returned acpi_status for failure instead. > >> > >> Signed-off-by: Kelsey Skunberg > >> --- > >> Changes in v2: > >>- Fixed white space warnings and errors > > > > Applied to net-next. > > Wow did you even build test this? Reverted... > This patch has definitely been a mess, so thank you for your time and sticking with me here. I thought my build tests included these files, though discovered they did not. Since submitting v2, I was able to reproduce the same errors you listed and corrected the problem in v3. I also realized my .git/post-commit file needed to be fixed, so the white space problem in v1 should also not be a problem in the future. Please let me know if you notice anything else I can improve on. I will learn from my mistakes and really appreciate advice. Thank you again, David. Best Regards, Kelsey
Re: [PATCH v2] drivers: net: xgene: Remove acpi_has_method() calls
On Tue, Jul 23, 2019 at 05:56:04PM -0500, Bjorn Helgaas wrote: > On Tue, Jul 23, 2019 at 1:59 PM Kelsey Skunberg > wrote: > > > > acpi_evaluate_object will already return an error if the needed method > > does not exist. Remove unnecessary acpi_has_method() calls and check the > > returned acpi_status for failure instead. > > > diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > > b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > > index 6453fc2ebb1f..5d637b46b2bf 100644 > > --- a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > > +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > > @@ -437,6 +437,7 @@ static void xgene_sgmac_tx_disable(struct > > xgene_enet_pdata *p) > > static int xgene_enet_reset(struct xgene_enet_pdata *p) > > { > > struct device *dev = >pdev->dev; > > + acpi_status status; > > > > if (!xgene_ring_mgr_init(p)) > > return -ENODEV; > > @@ -460,14 +461,13 @@ static int xgene_enet_reset(struct xgene_enet_pdata > > *p) > > } > > } else { > > #ifdef CONFIG_ACPI > > - if (acpi_has_method(ACPI_HANDLE(>pdev->dev), "_RST")) > > - acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), > > -"_RST", NULL, NULL); > > - else if (acpi_has_method(ACPI_HANDLE(>pdev->dev), > > "_INI")) > > + status = acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), > > + "_RST", NULL, NULL); > > + if (ACPI_FAILURE(status)) { > > acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), > > "_INI", NULL, NULL); > > + } > > #endif > > - } > > Oops, I don't think you intended to remove that brace. > > If you haven't found it already, CONFIG_COMPILE_TEST is useful for > building things that wouldn't normally be buildable on your arch. Thank you very much for catching that. I did not know about CONFIG_COMPILE_TEST yet and that will be very useful. It's clear why my build test wasn't coming up with the same errors now. I know for future patches now and will certainly get this one fixed. Thank you again. -Kelsey
Re: [PATCH v2] drivers: net: xgene: Remove acpi_has_method() calls
On Tue, Jul 23, 2019 at 1:59 PM Kelsey Skunberg wrote: > > acpi_evaluate_object will already return an error if the needed method > does not exist. Remove unnecessary acpi_has_method() calls and check the > returned acpi_status for failure instead. > diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > index 6453fc2ebb1f..5d637b46b2bf 100644 > --- a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c > @@ -437,6 +437,7 @@ static void xgene_sgmac_tx_disable(struct > xgene_enet_pdata *p) > static int xgene_enet_reset(struct xgene_enet_pdata *p) > { > struct device *dev = >pdev->dev; > + acpi_status status; > > if (!xgene_ring_mgr_init(p)) > return -ENODEV; > @@ -460,14 +461,13 @@ static int xgene_enet_reset(struct xgene_enet_pdata *p) > } > } else { > #ifdef CONFIG_ACPI > - if (acpi_has_method(ACPI_HANDLE(>pdev->dev), "_RST")) > - acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), > -"_RST", NULL, NULL); > - else if (acpi_has_method(ACPI_HANDLE(>pdev->dev), "_INI")) > + status = acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), > + "_RST", NULL, NULL); > + if (ACPI_FAILURE(status)) { > acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), > "_INI", NULL, NULL); > + } > #endif > - } Oops, I don't think you intended to remove that brace. If you haven't found it already, CONFIG_COMPILE_TEST is useful for building things that wouldn't normally be buildable on your arch. > if (!p->port_id) { > xgene_enet_ecc_init(p);
Re: [PATCH v2] drivers: net: xgene: Remove acpi_has_method() calls
From: David Miller Date: Tue, 23 Jul 2019 14:06:46 -0700 (PDT) > From: Kelsey Skunberg > Date: Tue, 23 Jul 2019 12:58:11 -0600 > >> acpi_evaluate_object will already return an error if the needed method >> does not exist. Remove unnecessary acpi_has_method() calls and check the >> returned acpi_status for failure instead. >> >> Signed-off-by: Kelsey Skunberg >> --- >> Changes in v2: >> - Fixed white space warnings and errors > > Applied to net-next. Wow did you even build test this? Reverted... drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c: In function ‘xgene_enet_reset’: drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:480:13: error: invalid storage class for function ‘xgene_enet_cle_bypass’ static void xgene_enet_cle_bypass(struct xgene_enet_pdata *p, ^ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:480:1: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] static void xgene_enet_cle_bypass(struct xgene_enet_pdata *p, ^~ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:506:13: error: invalid storage class for function ‘xgene_enet_clear’ static void xgene_enet_clear(struct xgene_enet_pdata *pdata, ^~~~ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:522:13: error: invalid storage class for function ‘xgene_enet_shutdown’ static void xgene_enet_shutdown(struct xgene_enet_pdata *p) ^~~ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:532:13: error: invalid storage class for function ‘xgene_enet_link_state’ static void xgene_enet_link_state(struct work_struct *work) ^ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:563:13: error: invalid storage class for function ‘xgene_sgmac_enable_tx_pause’ static void xgene_sgmac_enable_tx_pause(struct xgene_enet_pdata *p, bool enable) ^~~ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:604:1: error: expected declaration or statement at end of input }; ^ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:599:29: warning: unused variable ‘xgene_sgport_ops’ [-Wunused-variable] const struct xgene_port_ops xgene_sgport_ops = { ^~~~ drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:582:28: warning: unused variable ‘xgene_sgmac_ops’ [-Wunused-variable] const struct xgene_mac_ops xgene_sgmac_ops = { ^~~ At top level: drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:437:12: warning: ‘xgene_enet_reset’ defined but not used [-Wunused-function] static int xgene_enet_reset(struct xgene_enet_pdata *p) ^~~~
Re: [PATCH v2] drivers: net: xgene: Remove acpi_has_method() calls
From: Kelsey Skunberg Date: Tue, 23 Jul 2019 12:58:11 -0600 > acpi_evaluate_object will already return an error if the needed method > does not exist. Remove unnecessary acpi_has_method() calls and check the > returned acpi_status for failure instead. > > Signed-off-by: Kelsey Skunberg > --- > Changes in v2: > - Fixed white space warnings and errors Applied to net-next.
[PATCH v2] drivers: net: xgene: Remove acpi_has_method() calls
acpi_evaluate_object will already return an error if the needed method does not exist. Remove unnecessary acpi_has_method() calls and check the returned acpi_status for failure instead. Signed-off-by: Kelsey Skunberg --- Changes in v2: - Fixed white space warnings and errors drivers/net/ethernet/apm/xgene/xgene_enet_hw.c| 9 - drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c | 10 +- drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c | 9 - 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c index 61a465097cb8..79924efd4ab7 100644 --- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c @@ -694,6 +694,7 @@ bool xgene_ring_mgr_init(struct xgene_enet_pdata *p) static int xgene_enet_reset(struct xgene_enet_pdata *pdata) { struct device *dev = >pdev->dev; + acpi_status status; if (!xgene_ring_mgr_init(pdata)) return -ENODEV; @@ -712,11 +713,9 @@ static int xgene_enet_reset(struct xgene_enet_pdata *pdata) udelay(5); } else { #ifdef CONFIG_ACPI - if (acpi_has_method(ACPI_HANDLE(>pdev->dev), "_RST")) { - acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), -"_RST", NULL, NULL); - } else if (acpi_has_method(ACPI_HANDLE(>pdev->dev), -"_INI")) { + status = acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), + "_RST", NULL, NULL); + if (ACPI_FAILURE(status)) { acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), "_INI", NULL, NULL); } diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c index 6453fc2ebb1f..5d637b46b2bf 100644 --- a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c @@ -437,6 +437,7 @@ static void xgene_sgmac_tx_disable(struct xgene_enet_pdata *p) static int xgene_enet_reset(struct xgene_enet_pdata *p) { struct device *dev = >pdev->dev; + acpi_status status; if (!xgene_ring_mgr_init(p)) return -ENODEV; @@ -460,14 +461,13 @@ static int xgene_enet_reset(struct xgene_enet_pdata *p) } } else { #ifdef CONFIG_ACPI - if (acpi_has_method(ACPI_HANDLE(>pdev->dev), "_RST")) - acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), -"_RST", NULL, NULL); - else if (acpi_has_method(ACPI_HANDLE(>pdev->dev), "_INI")) + status = acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), + "_RST", NULL, NULL); + if (ACPI_FAILURE(status)) { acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), "_INI", NULL, NULL); + } #endif - } if (!p->port_id) { xgene_enet_ecc_init(p); diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c b/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c index 133eb91c542e..78584089d76d 100644 --- a/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c @@ -380,6 +380,7 @@ static void xgene_xgmac_tx_disable(struct xgene_enet_pdata *pdata) static int xgene_enet_reset(struct xgene_enet_pdata *pdata) { struct device *dev = >pdev->dev; + acpi_status status; if (!xgene_ring_mgr_init(pdata)) return -ENODEV; @@ -393,11 +394,9 @@ static int xgene_enet_reset(struct xgene_enet_pdata *pdata) udelay(5); } else { #ifdef CONFIG_ACPI - if (acpi_has_method(ACPI_HANDLE(>pdev->dev), "_RST")) { - acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), -"_RST", NULL, NULL); - } else if (acpi_has_method(ACPI_HANDLE(>pdev->dev), - "_INI")) { + status = acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), + "_RST", NULL, NULL); + if (ACPI_FAILURE(status)) { acpi_evaluate_object(ACPI_HANDLE(>pdev->dev), "_INI", NULL, NULL); } -- 2.20.1