On Oct. Saturday 10 (41) 05:07 PM, Florian Fainelli wrote: > 2015-10-10 15:41 GMT-07:00 Vivien Didelot > <vivien.dide...@savoirfairelinux.com>: > > On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote: > >> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.dide...@savoirfairelinux.com > >> wrote: > >> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote: > >> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote: > >> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala > >> >> ><pjonn...@broadcom.com> wrote: > >> >> >> > >> >> >> > >> >> >>> -----Original Message----- > >> >> >>> From: sfel...@gmail.com [mailto:sfel...@gmail.com] > >> >> >>> Sent: Friday, October 09, 2015 7:53 AM > >> >> >>> To: netdev@vger.kernel.org > >> >> >>> Cc: da...@davemloft.net; j...@resnulli.us; > >> >> >>> siva.mannem....@gmail.com; > >> >> >>> Premkumar Jonnala; step...@networkplumber.org; > >> >> >>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com; > >> >> >>> vivien.dide...@savoirfairelinux.com > >> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting > >> >> >>> ageing_time down > >> >> >>> to switchdev > >> >> >>> > >> >> >>> From: Scott Feldman <sfel...@gmail.com> > >> >> >>> > >> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that > >> >> >>> don't > >> >> >>> support setting ageing_time (or setting bridge attrs in general). > >> >> >>> > >> >> >>> If push fails, don't update ageing_time in bridge and return err to > >> >> >>> user. > >> >> >>> > >> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now > >> >> >>> to > >> >> >>> recalabrate when to run gc_timer next, based on new ageing_time. > >> >> >>> > >> >> >>> Signed-off-by: Scott Feldman <sfel...@gmail.com> > >> >> >>> Signed-off-by: Jiri Pirko <j...@resnulli.us> > >> >> > > >> >> ><snip> > >> >> > > >> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time) > >> >> >>> +{ > >> >> >>> + struct switchdev_attr attr = { > >> >> >>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME, > >> >> >>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP, > >> >> >>> + .u.ageing_time = ageing_time, > >> >> >>> + }; > >> >> >>> + unsigned long t = clock_t_to_jiffies(ageing_time); > >> >> >>> + int err; > >> >> >>> + > >> >> >>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME) > >> >> >>> + return -ERANGE; > >> >> >>> + > >> >> >>> + err = switchdev_port_attr_set(br->dev, &attr); > >> >> >> > >> >> >> A thought - given that the ageing time is not a per-bridge-port > >> >> >> attr, why are we using a "port based api" > >> >> >> to pass the attribute down? May be I'm missing something here? > >> >> > > >> >> >I think Florian raised the same point earlier. Sigh, I think this > >> >> >should be addressed....v4 coming soon...thanks guys for keeping the > >> >> >standard high. > >> >> > >> >> Scott, can you tell us how do you want to address this? I like the > >> >> current implementation. > >> > > >> >Scott, didn't you have a plan to add a struct device for the parent of > >> >switchdev ports? > >> > > >> >I think it would be good to introduce such device with an helper to > >> >retrieve this upper parent, and move the switchdev ops to this guy. > >> > > >> >So switchdev drivers may implement such API calls: > >> > > >> > .obj_add(struct device *swdev, struct switchdev_obj *obj); > >> > > >> > .port_obj_add(struct device *swdev, struct net_device *port, > >> > struct switchdev_obj *obj); > >> > > >> >Then switchdev code may have a parent API and the current port API may > >> >look like this: > >> > > >> > int switchdev_port_obj_add(struct net_device *dev, > >> > struct switchdev_obj *obj) > >> > { > >> > struct device *swdev = switchdev_get_parent(dev); > >> > int err = -EOPNOTSUPP; > >> > > >> > if (swdev && swdev->switchdev_ops && > >> > swdev->switchdev_ops->port_obj_add) > >> > err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj); > >> > > >> > return err; > >> > } > >> > >> Fro the record, I don't see any reason for this "device". It would just > >> introduce unnecessary complexicity. So far, we are fine without it. > > > > I wouldn't say that. I beleive that an Ethernet switch deserves its > > struct device in the tree, since it is a physical chip, like any other. > > Agreed, but gating these patches because we do not yet have a device > driver model for an Ethernet switch outside of its individual ports > does not seem like it hurts the current patch series, nor the existing > model (and future).
Sure, I didn't mean to NAK the patch with this comment, I just wrote it because we raised a concern about an API higher than the port level. > > > > Configuring it through one of its port (net_device) is fine, since you > > want to change the port behavior, and Linux config is on per-port basis. > > > > But the complexity is already introduced in the struct net_device with > > the switchdev_ops. These ops really belong to the parent device, not to > > all of its ports. > > I am not sure if complexity is the correct term here, bloat (to some > extent) maybe, since with what you are suggesting we could save one > set of function pointers per-port, and instead move that to a > global/switch-wide device implementing these operations. In essence, > there will be per-port switchdev_ops, bridge-specific, and maybe in > the future switch device specific. Exact. I think that a switch net_device port should just have a pointer or something to its parent device (the switch) to query its operations. > > > > Ideally a switch device would be registered with this set of operations, > > creates its net_devices, and will be accessible from a port net_device > > through a netdev helper function. > > I think the core of the discussion for a proper Ethernet switch device > model is precisely whether we want to have a special network device to > configure the switch as a whole. It sure would represent one facet of > the switch device, but not everything else for which we are still > trying to find out what that is. I am not even convinced that a switch chip must be represented in the Linux tree by a net_device. That's basically a chip exposing a bench of registers to configure and expose not only net interfaces, but also temperature sensors, and even GPIO sometimes. Thanks, -v -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
