| Message ID | 1449841719-5756-1-git-send-email-zajec5@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
On 11/12/2015 14:48, Rafał Miłecki wrote: > @@ -251,6 +279,7 @@ static struct switch_attr default_port[] = { > .description = "Get port link information", > .set = NULL, you forgot to delete this line
Hi Rafał, in addition to the comment i made last night, i found another possible issue. see below. On 11/12/2015 14:48, Rafał Miłecki wrote: > Some switches can force link speed for a port. Let's add API that will > allow driver to export this feature. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > .../linux/generic/files/drivers/net/phy/swconfig.c | 29 ++++++++++++++++++++++ > target/linux/generic/files/include/linux/switch.h | 2 ++ > 2 files changed, 31 insertions(+) > > diff --git a/target/linux/generic/files/drivers/net/phy/swconfig.c b/target/linux/generic/files/drivers/net/phy/swconfig.c > index 6bb3be1..58d6cf5 100644 > --- a/target/linux/generic/files/drivers/net/phy/swconfig.c > +++ b/target/linux/generic/files/drivers/net/phy/swconfig.c > @@ -187,6 +187,34 @@ swconfig_get_link(struct switch_dev *dev, const struct switch_attr *attr, > } > > static int > +swconfig_set_link(struct switch_dev *dev, const struct switch_attr *attr, > + struct switch_val *val) > +{ > + enum switch_port_speed speed; > + const char *s = val->value.s; > + int len; > + > + if (val->port_vlan >= dev->ports) > + return -EINVAL; > + > + if (!dev->ops->set_port_link) > + return -EOPNOTSUPP; > + > + len = strchrnul(s, ' ') - s; > + if (!strncmp(s, "10", len)) { > + speed = SWITCH_PORT_SPEED_10; > + } else if (!strncmp(s, "100", len)) { > + speed = SWITCH_PORT_SPEED_100; > + } else if (!strncmp(s, "1000", len)) { > + speed = SWITCH_PORT_SPEED_1000; > + } else { > + speed = SWITCH_PORT_SPEED_UNKNOWN; > + } > + > + return dev->ops->set_port_link(dev, val->port_vlan, speed); this makes the assumption, that all drivers always add the set_port_link() callback and uses it unconditionally. i am impartial on how this is solved in detail as long as some kind of check is added John > +} > + > +static int > swconfig_apply_config(struct switch_dev *dev, const struct switch_attr *attr, > struct switch_val *val) > { > @@ -251,6 +279,7 @@ static struct switch_attr default_port[] = { > .description = "Get port link information", > .set = NULL, > .get = swconfig_get_link, > + .set = swconfig_set_link, > } > }; > > diff --git a/target/linux/generic/files/include/linux/switch.h b/target/linux/generic/files/include/linux/switch.h > index 4291364..e21fb05 100644 > --- a/target/linux/generic/files/include/linux/switch.h > +++ b/target/linux/generic/files/include/linux/switch.h > @@ -95,6 +95,8 @@ struct switch_dev_ops { > > int (*get_port_link)(struct switch_dev *dev, int port, > struct switch_port_link *link); > + int (*set_port_link)(struct switch_dev *dev, int port, > + enum switch_port_speed speed); > int (*get_port_stats)(struct switch_dev *dev, int port, > struct switch_port_stats *stats); > }; >
On 12 December 2015 at 07:54, John Crispin <blogic@openwrt.org> wrote: > Hi Rafał, > > in addition to the comment i made last night, i found another possible > issue. see below. > > On 11/12/2015 14:48, Rafał Miłecki wrote: >> Some switches can force link speed for a port. Let's add API that will >> allow driver to export this feature. >> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >> --- >> .../linux/generic/files/drivers/net/phy/swconfig.c | 29 ++++++++++++++++++++++ >> target/linux/generic/files/include/linux/switch.h | 2 ++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/target/linux/generic/files/drivers/net/phy/swconfig.c b/target/linux/generic/files/drivers/net/phy/swconfig.c >> index 6bb3be1..58d6cf5 100644 >> --- a/target/linux/generic/files/drivers/net/phy/swconfig.c >> +++ b/target/linux/generic/files/drivers/net/phy/swconfig.c >> @@ -187,6 +187,34 @@ swconfig_get_link(struct switch_dev *dev, const struct switch_attr *attr, >> } >> >> static int >> +swconfig_set_link(struct switch_dev *dev, const struct switch_attr *attr, >> + struct switch_val *val) >> +{ >> + enum switch_port_speed speed; >> + const char *s = val->value.s; >> + int len; >> + >> + if (val->port_vlan >= dev->ports) >> + return -EINVAL; >> + >> + if (!dev->ops->set_port_link) >> + return -EOPNOTSUPP; >> + >> + len = strchrnul(s, ' ') - s; >> + if (!strncmp(s, "10", len)) { >> + speed = SWITCH_PORT_SPEED_10; >> + } else if (!strncmp(s, "100", len)) { >> + speed = SWITCH_PORT_SPEED_100; >> + } else if (!strncmp(s, "1000", len)) { >> + speed = SWITCH_PORT_SPEED_1000; >> + } else { >> + speed = SWITCH_PORT_SPEED_UNKNOWN; >> + } >> + >> + return dev->ops->set_port_link(dev, val->port_vlan, speed); > > this makes the assumption, that all drivers always add the > set_port_link() callback and uses it unconditionally. i am impartial on > how this is solved in detail as long as some kind of check is added You had to miss if (!dev->ops->set_port_link) part :)
On 12/12/2015 09:04, Rafał Miłecki wrote: > On 12 December 2015 at 07:54, John Crispin <blogic@openwrt.org> wrote: >> Hi Rafał, >> >> in addition to the comment i made last night, i found another possible >> issue. see below. >> >> On 11/12/2015 14:48, Rafał Miłecki wrote: >>> Some switches can force link speed for a port. Let's add API that will >>> allow driver to export this feature. >>> >>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >>> --- >>> .../linux/generic/files/drivers/net/phy/swconfig.c | 29 ++++++++++++++++++++++ >>> target/linux/generic/files/include/linux/switch.h | 2 ++ >>> 2 files changed, 31 insertions(+) >>> >>> diff --git a/target/linux/generic/files/drivers/net/phy/swconfig.c b/target/linux/generic/files/drivers/net/phy/swconfig.c >>> index 6bb3be1..58d6cf5 100644 >>> --- a/target/linux/generic/files/drivers/net/phy/swconfig.c >>> +++ b/target/linux/generic/files/drivers/net/phy/swconfig.c >>> @@ -187,6 +187,34 @@ swconfig_get_link(struct switch_dev *dev, const struct switch_attr *attr, >>> } >>> >>> static int >>> +swconfig_set_link(struct switch_dev *dev, const struct switch_attr *attr, >>> + struct switch_val *val) >>> +{ >>> + enum switch_port_speed speed; >>> + const char *s = val->value.s; >>> + int len; >>> + >>> + if (val->port_vlan >= dev->ports) >>> + return -EINVAL; >>> + >>> + if (!dev->ops->set_port_link) >>> + return -EOPNOTSUPP; >>> + >>> + len = strchrnul(s, ' ') - s; >>> + if (!strncmp(s, "10", len)) { >>> + speed = SWITCH_PORT_SPEED_10; >>> + } else if (!strncmp(s, "100", len)) { >>> + speed = SWITCH_PORT_SPEED_100; >>> + } else if (!strncmp(s, "1000", len)) { >>> + speed = SWITCH_PORT_SPEED_1000; >>> + } else { >>> + speed = SWITCH_PORT_SPEED_UNKNOWN; >>> + } >>> + >>> + return dev->ops->set_port_link(dev, val->port_vlan, speed); >> >> this makes the assumption, that all drivers always add the >> set_port_link() callback and uses it unconditionally. i am impartial on >> how this is solved in detail as long as some kind of check is added > > You had to miss > if (!dev->ops->set_port_link) > part :) > i did indeed ...
Hi Rafał, > int (*get_port_link)(struct switch_dev *dev, int port, > struct switch_port_link *link); > + int (*set_port_link)(struct switch_dev *dev, int port, > + enum switch_port_speed speed); this creates an assymetric API. I think the prototype of the set function should be int (*set_port_link)(struct switch_dev *dev, int port, struct switch_port_link *link); to allow setting other parameters that are part of struct switch_port_link, like duplex or flow control. I'd be rather happy to disable crappy ethernet flow control on my router... API should be: If switch_port_link.aneg is enabled, the switch driver should advertise the capabilities as set in the struct. May be this requires making switch_port_speed a bit field for the allowed speeds. If switch_port_link.aneg is disabled, the driver should disable autonegotation and force the parameters set. If anything is set the switch does not support, the driver should return -EINVAL. Stefan
On 13 December 2015 at 13:40, Stefan Rompf <stefan@loplof.de> wrote: >> int (*get_port_link)(struct switch_dev *dev, int port, >> struct switch_port_link *link); >> + int (*set_port_link)(struct switch_dev *dev, int port, >> + enum switch_port_speed speed); > > this creates an assymetric API. I think the prototype of the set function > should be > > int (*set_port_link)(struct switch_dev *dev, int port, > struct switch_port_link *link); > > to allow setting other parameters that are part of struct switch_port_link, > like duplex or flow control. I'd be rather happy to disable crappy ethernet > flow control on my router... Thanks, I'll work on this. For the future: it's preferred to reply to the latest patch version. Your comment isn't visible for the V1 in patchwork: https://patchwork.ozlabs.org/patch/556017/
diff --git a/target/linux/generic/files/drivers/net/phy/swconfig.c b/target/linux/generic/files/drivers/net/phy/swconfig.c index 6bb3be1..58d6cf5 100644 --- a/target/linux/generic/files/drivers/net/phy/swconfig.c +++ b/target/linux/generic/files/drivers/net/phy/swconfig.c @@ -187,6 +187,34 @@ swconfig_get_link(struct switch_dev *dev, const struct switch_attr *attr, } static int +swconfig_set_link(struct switch_dev *dev, const struct switch_attr *attr, + struct switch_val *val) +{ + enum switch_port_speed speed; + const char *s = val->value.s; + int len; + + if (val->port_vlan >= dev->ports) + return -EINVAL; + + if (!dev->ops->set_port_link) + return -EOPNOTSUPP; + + len = strchrnul(s, ' ') - s; + if (!strncmp(s, "10", len)) { + speed = SWITCH_PORT_SPEED_10; + } else if (!strncmp(s, "100", len)) { + speed = SWITCH_PORT_SPEED_100; + } else if (!strncmp(s, "1000", len)) { + speed = SWITCH_PORT_SPEED_1000; + } else { + speed = SWITCH_PORT_SPEED_UNKNOWN; + } + + return dev->ops->set_port_link(dev, val->port_vlan, speed); +} + +static int swconfig_apply_config(struct switch_dev *dev, const struct switch_attr *attr, struct switch_val *val) { @@ -251,6 +279,7 @@ static struct switch_attr default_port[] = { .description = "Get port link information", .set = NULL, .get = swconfig_get_link, + .set = swconfig_set_link, } }; diff --git a/target/linux/generic/files/include/linux/switch.h b/target/linux/generic/files/include/linux/switch.h index 4291364..e21fb05 100644 --- a/target/linux/generic/files/include/linux/switch.h +++ b/target/linux/generic/files/include/linux/switch.h @@ -95,6 +95,8 @@ struct switch_dev_ops { int (*get_port_link)(struct switch_dev *dev, int port, struct switch_port_link *link); + int (*set_port_link)(struct switch_dev *dev, int port, + enum switch_port_speed speed); int (*get_port_stats)(struct switch_dev *dev, int port, struct switch_port_stats *stats); };
Some switches can force link speed for a port. Let's add API that will allow driver to export this feature. Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- .../linux/generic/files/drivers/net/phy/swconfig.c | 29 ++++++++++++++++++++++ target/linux/generic/files/include/linux/switch.h | 2 ++ 2 files changed, 31 insertions(+)