2019-08-13 08:57:44

by Ioana Ciornei

Subject: [PATCH v2 10/10] staging: fsl-dpaa2/ethsw: do not force user to bring interface down

Link settings can be changed only when the interface is down. Disable
and re-enable the interface, if necessary, behind the scenes so that we do
not force users to an if down/up sequence.

Reported-by: Andrew Lunn <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
Changes in v2:
- added Reported-by tag

drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 32 ++++++++++++++++++-------
1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
index 0f9f8345e534..99d658fefa14 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
@@ -88,16 +88,21 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
const struct ethtool_link_ksettings *link_ksettings)
{
struct ethsw_port_priv *port_priv = netdev_priv(netdev);
+ struct ethsw_core *ethsw = port_priv->ethsw_data;
struct dpsw_link_cfg cfg = {0};
- int err = 0;
-
- /* Due to a temporary MC limitation, the DPSW port must be down
- * in order to be able to change link settings. Taking steps to let
- * the user know that.
- */
- if (netif_running(netdev)) {
- netdev_info(netdev, "Sorry, interface must be brought down first.\n");
- return -EACCES;
+ bool if_running;
+ int err = 0, ret;
+
+ /* Interface needs to be down to change link settings */
+ if_running = netif_running(netdev);
+ if (if_running) {
+ err = dpsw_if_disable(ethsw->mc_io, 0,
+ ethsw->dpsw_handle,
+ port_priv->idx);
+ if (err) {
+ netdev_err(netdev, "dpsw_if_disable err %d\n", err);
+ return err;
+ }
}

cfg.rate = link_ksettings->base.speed;
@@ -115,6 +120,15 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
port_priv->idx,
&cfg);

+ if (if_running) {
+ ret = dpsw_if_enable(ethsw->mc_io, 0,
+ ethsw->dpsw_handle,
+ port_priv->idx);
+ if (ret) {
+ return ret;
+ netdev_err(netdev, "dpsw_if_enable err %d\n", ret);
+ }
+ }
return err;
}

--
1.9.1

2019-08-13 09:40:36

by Razvan Stefanescu

Subject: Re: [PATCH v2 10/10] staging: fsl-dpaa2/ethsw: do not force user to bring interface down

On 13/08/2019 11:54, Ioana Ciornei wrote:
> Link settings can be changed only when the interface is down. Disable
> and re-enable the interface, if necessary, behind the scenes so that we do
> not force users to an if down/up sequence.
>
> Reported-by: Andrew Lunn <[email protected]>
> Signed-off-by: Ioana Ciornei <[email protected]>
> ---
> Changes in v2:
> - added Reported-by tag
>
> drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 32 ++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
> index 0f9f8345e534..99d658fefa14 100644
> --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
> +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
> @@ -88,16 +88,21 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
> const struct ethtool_link_ksettings *link_ksettings)
> {
> struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> + struct ethsw_core *ethsw = port_priv->ethsw_data;
> struct dpsw_link_cfg cfg = {0};
> - int err = 0;
> -
> - /* Due to a temporary MC limitation, the DPSW port must be down
> - * in order to be able to change link settings. Taking steps to let
> - * the user know that.
> - */
> - if (netif_running(netdev)) {
> - netdev_info(netdev, "Sorry, interface must be brought down first.\n");
> - return -EACCES;
> + bool if_running;
> + int err = 0, ret;
> +
> + /* Interface needs to be down to change link settings */
> + if_running = netif_running(netdev);
> + if (if_running) {
> + err = dpsw_if_disable(ethsw->mc_io, 0,
> + ethsw->dpsw_handle,
> + port_priv->idx);
> + if (err) {
> + netdev_err(netdev, "dpsw_if_disable err %d\n", err);
> + return err;
> + }
> }
>
> cfg.rate = link_ksettings->base.speed;
> @@ -115,6 +120,15 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
> port_priv->idx,
> &cfg);
>
> + if (if_running) {
> + ret = dpsw_if_enable(ethsw->mc_io, 0,
> + ethsw->dpsw_handle,
> + port_priv->idx);
> + if (ret) {
> + return ret;
> + netdev_err(netdev, "dpsw_if_enable err %d\n", ret);
Hello,

These last two lines need to be swapped.

Best regards,
Razvan

2019-08-13 09:46:19

by Ioana Ciornei

Subject: Re: [PATCH v2 10/10] staging: fsl-dpaa2/ethsw: do not force user to bring interface down

On 8/13/19 12:38 PM, [email protected] wrote:
>
>
> On 13/08/2019 11:54, Ioana Ciornei wrote:
>> Link settings can be changed only when the interface is down. Disable
>> and re-enable the interface, if necessary, behind the scenes so that we do
>> not force users to an if down/up sequence.
>>
>> Reported-by: Andrew Lunn <[email protected]>
>> Signed-off-by: Ioana Ciornei <[email protected]>
>> ---
>> Changes in v2:
>> - added Reported-by tag
>>
>> drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 32 ++++++++++++++++++-------
>> 1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
>> index 0f9f8345e534..99d658fefa14 100644
>> --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
>> +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
>> @@ -88,16 +88,21 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
>> const struct ethtool_link_ksettings *link_ksettings)
>> {
>> struct ethsw_port_priv *port_priv = netdev_priv(netdev);
>> + struct ethsw_core *ethsw = port_priv->ethsw_data;
>> struct dpsw_link_cfg cfg = {0};
>> - int err = 0;
>> -
>> - /* Due to a temporary MC limitation, the DPSW port must be down
>> - * in order to be able to change link settings. Taking steps to let
>> - * the user know that.
>> - */
>> - if (netif_running(netdev)) {
>> - netdev_info(netdev, "Sorry, interface must be brought down first.\n");
>> - return -EACCES;
>> + bool if_running;
>> + int err = 0, ret;
>> +
>> + /* Interface needs to be down to change link settings */
>> + if_running = netif_running(netdev);
>> + if (if_running) {
>> + err = dpsw_if_disable(ethsw->mc_io, 0,
>> + ethsw->dpsw_handle,
>> + port_priv->idx);
>> + if (err) {
>> + netdev_err(netdev, "dpsw_if_disable err %d\n", err);
>> + return err;
>> + }
>> }
>>
>> cfg.rate = link_ksettings->base.speed;
>> @@ -115,6 +120,15 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
>> port_priv->idx,
>> &cfg);
>>
>> + if (if_running) {
>> + ret = dpsw_if_enable(ethsw->mc_io, 0,
>> + ethsw->dpsw_handle,
>> + port_priv->idx);
>> + if (ret) {
>> + return ret;
>> + netdev_err(netdev, "dpsw_if_enable err %d\n", ret);
> Hello, >
> These last two lines need to be swapped.
>
> Best regards,
> Razvan
>

Oops, my bad. Will fix.

Thanks for pointing this out,
Ioana