| Message ID | 20211204182858.1052710-6-colin.foster@in-advantage.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | prepare ocelot for external interface control | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 82 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Sat, 4 Dec 2021 10:28:58 -0800 Colin Foster wrote: > Expose ocelot_wm functions so they can be shared with other drivers. > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Yeah.. but there are no in-tree users of these. What's the story? I see Vladimir reviewed this so presumably we trust that the users will materialize rather quickly?
On Mon, Dec 06, 2021 at 06:09:22PM -0800, Jakub Kicinski wrote: > On Sat, 4 Dec 2021 10:28:58 -0800 Colin Foster wrote: > > Expose ocelot_wm functions so they can be shared with other drivers. > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Yeah.. but there are no in-tree users of these. What's the story? > > I see Vladimir reviewed this so presumably we trust that the users > will materialize rather quickly? Thank you for highlighting this. Vladimir told me recently over the phylink get_interfaces vs get_caps change for DSA, and I quote: David who applied your patch can correct me, but my understanding from the little time I've spent on netdev is that dead code isn't a candidate for getting accepted into the tree, even more so in the last few days before the merge window, from where it got into v5.16-rc1. ... So yes, I take issue with that as a matter of principle, I very much expect that a kernel developer of your experience does not set a precedent and a pretext for people who submit various shady stuff to the kernel just to make their downstream life easier. This sounds very much like double-standards, especially as Vladimir reviewed this. I'm not going to be spiteful NAK these patches, because we all need to get along with each other. I realise that it is sometimes useful to get code merged that facilitates or aids further development - provided that development is submitted in a timely manner.
On Tue, Dec 07, 2021 at 11:48:52AM +0000, Russell King (Oracle) wrote: > On Mon, Dec 06, 2021 at 06:09:22PM -0800, Jakub Kicinski wrote: > > On Sat, 4 Dec 2021 10:28:58 -0800 Colin Foster wrote: > > > Expose ocelot_wm functions so they can be shared with other drivers. > > > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Yeah.. but there are no in-tree users of these. What's the story? > > > > I see Vladimir reviewed this so presumably we trust that the users > > will materialize rather quickly? > > Thank you for highlighting this. > > Vladimir told me recently over the phylink get_interfaces vs get_caps > change for DSA, and I quote: > > David who applied your patch can correct me, but my understanding from > the little time I've spent on netdev is that dead code isn't a candidate > for getting accepted into the tree, even more so in the last few days > before the merge window, from where it got into v5.16-rc1. > ... > So yes, I take issue with that as a matter of principle, I very much > expect that a kernel developer of your experience does not set a > precedent and a pretext for people who submit various shady stuff to the > kernel just to make their downstream life easier. > > This sounds very much like double-standards, especially as Vladimir > reviewed this. > > I'm not going to be spiteful NAK these patches, because we all need to > get along with each other. I realise that it is sometimes useful to get > code merged that facilitates or aids further development - provided > that development is submitted in a timely manner. I'm not taking this as a spiteful comment either, it is a very fair point. Colin had previously submitted this as part of a 23-patch series and it was me who suggested that this change could go in as part of preparation work right away: https://patchwork.kernel.org/project/netdevbpf/cover/20211116062328.1949151-1-colin.foster@in-advantage.com/#24596529 I didn't realize that in doing so with this particular change, we would end up having some symbols exported by the ocelot switch lib that aren't yet in use by other drivers. So yes, this would have to go in at the same time as the driver submission itself.
On Tue, 7 Dec 2021 12:11:22 +0000 Vladimir Oltean wrote: > On Tue, Dec 07, 2021 at 11:48:52AM +0000, Russell King (Oracle) wrote: > > Thank you for highlighting this. > > > > Vladimir told me recently over the phylink get_interfaces vs get_caps > > change for DSA, and I quote: > > > > David who applied your patch can correct me, but my understanding from > > the little time I've spent on netdev is that dead code isn't a candidate > > for getting accepted into the tree, even more so in the last few days > > before the merge window, from where it got into v5.16-rc1. > > ... > > So yes, I take issue with that as a matter of principle, I very much > > expect that a kernel developer of your experience does not set a > > precedent and a pretext for people who submit various shady stuff to the > > kernel just to make their downstream life easier. > > > > This sounds very much like double-standards, especially as Vladimir > > reviewed this. > > > > I'm not going to be spiteful NAK these patches, because we all need to > > get along with each other. I realise that it is sometimes useful to get > > code merged that facilitates or aids further development - provided > > that development is submitted in a timely manner. > > I'm not taking this as a spiteful comment either, it is a very fair point. > Colin had previously submitted this as part of a 23-patch series and it > was me who suggested that this change could go in as part of preparation > work right away: > https://patchwork.kernel.org/project/netdevbpf/cover/20211116062328.1949151-1-colin.foster@in-advantage.com/#24596529 > I didn't realize that in doing so with this particular change, we would > end up having some symbols exported by the ocelot switch lib that aren't > yet in use by other drivers. So yes, this would have to go in at the > same time as the driver submission itself. I don't know the dependencies here (there are also pinctrl patches in the linked series) so I'll defer to you, if there is a reason to merge the unused symbols it needs to be spelled out, otherwise let's drop the last patch for now.
On Tue, Dec 07, 2021 at 07:26:52AM -0800, Jakub Kicinski wrote: > On Tue, 7 Dec 2021 12:11:22 +0000 Vladimir Oltean wrote: > > On Tue, Dec 07, 2021 at 11:48:52AM +0000, Russell King (Oracle) wrote: > > > Thank you for highlighting this. > > > > > > Vladimir told me recently over the phylink get_interfaces vs get_caps > > > change for DSA, and I quote: > > > > > > David who applied your patch can correct me, but my understanding from > > > the little time I've spent on netdev is that dead code isn't a candidate > > > for getting accepted into the tree, even more so in the last few days > > > before the merge window, from where it got into v5.16-rc1. > > > ... > > > So yes, I take issue with that as a matter of principle, I very much > > > expect that a kernel developer of your experience does not set a > > > precedent and a pretext for people who submit various shady stuff to the > > > kernel just to make their downstream life easier. > > > > > > This sounds very much like double-standards, especially as Vladimir > > > reviewed this. > > > > > > I'm not going to be spiteful NAK these patches, because we all need to > > > get along with each other. I realise that it is sometimes useful to get > > > code merged that facilitates or aids further development - provided > > > that development is submitted in a timely manner. > > > > I'm not taking this as a spiteful comment either, it is a very fair point. > > Colin had previously submitted this as part of a 23-patch series and it > > was me who suggested that this change could go in as part of preparation > > work right away: > > https://patchwork.kernel.org/project/netdevbpf/cover/20211116062328.1949151-1-colin.foster@in-advantage.com/#24596529 > > I didn't realize that in doing so with this particular change, we would > > end up having some symbols exported by the ocelot switch lib that aren't > > yet in use by other drivers. So yes, this would have to go in at the > > same time as the driver submission itself. > > I don't know the dependencies here (there are also pinctrl patches > in the linked series) so I'll defer to you, if there is a reason to > merge the unused symbols it needs to be spelled out, otherwise let's > drop the last patch for now. I don't think there's any problem with dropping the last patch for now, as that's the safer thing to do (Colin?), but just let us know whether you prefer Colin to resend a 4-patch series, or you can pick this series up without the last one.
On Tue, 7 Dec 2021 15:30:12 +0000 Vladimir Oltean wrote: > On Tue, Dec 07, 2021 at 07:26:52AM -0800, Jakub Kicinski wrote: > > On Tue, 7 Dec 2021 12:11:22 +0000 Vladimir Oltean wrote: > > > I'm not taking this as a spiteful comment either, it is a very fair point. > > > Colin had previously submitted this as part of a 23-patch series and it > > > was me who suggested that this change could go in as part of preparation > > > work right away: > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211116062328.1949151-1-colin.foster@in-advantage.com/#24596529 > > > I didn't realize that in doing so with this particular change, we would > > > end up having some symbols exported by the ocelot switch lib that aren't > > > yet in use by other drivers. So yes, this would have to go in at the > > > same time as the driver submission itself. > > > > I don't know the dependencies here (there are also pinctrl patches > > in the linked series) so I'll defer to you, if there is a reason to > > merge the unused symbols it needs to be spelled out, otherwise let's > > drop the last patch for now. > > I don't think there's any problem with dropping the last patch for now, > as that's the safer thing to do (Colin?), but just let us know whether > you prefer Colin to resend a 4-patch series, or you can pick this series > up without the last one. Repost once it's confirmed that's the right course of action. I'll merge it right away.
On Tue, Dec 07, 2021 at 07:39:00AM -0800, Jakub Kicinski wrote: > On Tue, 7 Dec 2021 15:30:12 +0000 Vladimir Oltean wrote: > > On Tue, Dec 07, 2021 at 07:26:52AM -0800, Jakub Kicinski wrote: > > > On Tue, 7 Dec 2021 12:11:22 +0000 Vladimir Oltean wrote: > > > > I'm not taking this as a spiteful comment either, it is a very fair point. > > > > Colin had previously submitted this as part of a 23-patch series and it > > > > was me who suggested that this change could go in as part of preparation > > > > work right away: > > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211116062328.1949151-1-colin.foster@in-advantage.com/#24596529 > > > > I didn't realize that in doing so with this particular change, we would > > > > end up having some symbols exported by the ocelot switch lib that aren't > > > > yet in use by other drivers. So yes, this would have to go in at the > > > > same time as the driver submission itself. > > > > > > I don't know the dependencies here (there are also pinctrl patches > > > in the linked series) so I'll defer to you, if there is a reason to > > > merge the unused symbols it needs to be spelled out, otherwise let's > > > drop the last patch for now. > > > > I don't think there's any problem with dropping the last patch for now, > > as that's the safer thing to do (Colin?), but just let us know whether > > you prefer Colin to resend a 4-patch series, or you can pick this series > > up without the last one. > > Repost once it's confirmed that's the right course of action. > I'll merge it right away. I should be able to repost today without the last patch. It is an easy, and resonable one to drag along with the larger ocelot_spi patches I have in progress. And I appreciate all the insight and feedback from everyone. Thanks for helping me keep keep me on track!
diff --git a/drivers/net/ethernet/mscc/ocelot_devlink.c b/drivers/net/ethernet/mscc/ocelot_devlink.c index b8737efd2a85..d9ea75a14f2f 100644 --- a/drivers/net/ethernet/mscc/ocelot_devlink.c +++ b/drivers/net/ethernet/mscc/ocelot_devlink.c @@ -487,6 +487,37 @@ static void ocelot_watermark_init(struct ocelot *ocelot) ocelot_setup_sharing_watermarks(ocelot); } +/* Watermark encode + * Bit 8: Unit; 0:1, 1:16 + * Bit 7-0: Value to be multiplied with unit + */ +u16 ocelot_wm_enc(u16 value) +{ + WARN_ON(value >= 16 * BIT(8)); + + if (value >= BIT(8)) + return BIT(8) | (value / 16); + + return value; +} +EXPORT_SYMBOL(ocelot_wm_enc); + +u16 ocelot_wm_dec(u16 wm) +{ + if (wm & BIT(8)) + return (wm & GENMASK(7, 0)) * 16; + + return wm; +} +EXPORT_SYMBOL(ocelot_wm_dec); + +void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse) +{ + *inuse = (val & GENMASK(23, 12)) >> 12; + *maxuse = val & GENMASK(11, 0); +} +EXPORT_SYMBOL(ocelot_wm_stat); + /* Pool size and type are fixed up at runtime. Keeping this structure to * look up the cell size multipliers. */ diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index 2db59060f5ab..6f2d1d58a1ed 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -306,34 +306,6 @@ static int ocelot_reset(struct ocelot *ocelot) return 0; } -/* Watermark encode - * Bit 8: Unit; 0:1, 1:16 - * Bit 7-0: Value to be multiplied with unit - */ -static u16 ocelot_wm_enc(u16 value) -{ - WARN_ON(value >= 16 * BIT(8)); - - if (value >= BIT(8)) - return BIT(8) | (value / 16); - - return value; -} - -static u16 ocelot_wm_dec(u16 wm) -{ - if (wm & BIT(8)) - return (wm & GENMASK(7, 0)) * 16; - - return wm; -} - -static void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse) -{ - *inuse = (val & GENMASK(23, 12)) >> 12; - *maxuse = val & GENMASK(11, 0); -} - static const struct ocelot_ops ocelot_ops = { .reset = ocelot_reset, .wm_enc = ocelot_wm_enc, diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index 33f2e8c9e88b..0ac0ef116032 100644 --- a/include/soc/mscc/ocelot.h +++ b/include/soc/mscc/ocelot.h @@ -806,6 +806,11 @@ void ocelot_deinit(struct ocelot *ocelot); void ocelot_init_port(struct ocelot *ocelot, int port); void ocelot_deinit_port(struct ocelot *ocelot, int port); +/* Watermark interface */ +u16 ocelot_wm_enc(u16 value); +u16 ocelot_wm_dec(u16 wm); +void ocelot_wm_stat(u32 val, u32 *inuse, u32 *maxuse); + /* DSA callbacks */ void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data); void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data);