Each instance of an ocelot struct has the ocelot_vcap_props structure being
referenced. During initialization (ocelot_init), these vcap_props are
detected and the structure contents are modified.
In the case of the standard ocelot driver, there will probably only be one
instance of struct ocelot, since it is part of the chip.
For the Felix driver, there could be multiple instances of struct ocelot.
In that scenario, the second time ocelot_init would get called, it would
corrupt what had been done in the first call because they both reference
*ocelot->vcap. Both of these instances were assigned the same memory
location.
Move this vcap_props memory to within struct ocelot, so that each instance
can modify the structure to their heart's content without corrupting other
instances.
Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
constants")
Signed-off-by: Colin Foster <[email protected]>
---
drivers/net/dsa/ocelot/felix.c | 3 +-
drivers/net/dsa/ocelot/felix.h | 2 +-
drivers/net/dsa/ocelot/felix_vsc9959.c | 2 +-
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++--
include/soc/mscc/ocelot.h | 34 +++++++++++++++++++++-
include/soc/mscc/ocelot_vcap.h | 32 --------------------
6 files changed, 40 insertions(+), 38 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 9e28219b223d..f6a1e8e90bda 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1199,7 +1199,8 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
ocelot->stats_layout = felix->info->stats_layout;
ocelot->num_stats = felix->info->num_stats;
ocelot->num_mact_rows = felix->info->num_mact_rows;
- ocelot->vcap = felix->info->vcap;
+ memcpy(&ocelot->vcap, felix->info->vcap,
+ OCELOT_NUM_VCAP_BLOCKS * sizeof(*felix->info->vcap));
ocelot->vcap_pol.base = felix->info->vcap_pol_base;
ocelot->vcap_pol.max = felix->info->vcap_pol_max;
ocelot->vcap_pol.base2 = felix->info->vcap_pol_base2;
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index d6cf5e5a48c5..fb928c8bf544 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -27,7 +27,7 @@ struct felix_info {
unsigned int num_stats;
int num_ports;
int num_tx_queues;
- struct ocelot_vcap_props *vcap;
+ const struct ocelot_vcap_props *vcap;
u16 vcap_pol_base;
u16 vcap_pol_max;
u16 vcap_pol_base2;
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index a60dbedc1b1c..ddf4e8a9905c 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -845,7 +845,7 @@ static struct vcap_field vsc9959_vcap_is2_actions[] = {
[VCAP_IS2_ACT_HIT_CNT] = { 44, 32},
};
-static struct ocelot_vcap_props vsc9959_vcap_props[] = {
+static const struct ocelot_vcap_props vsc9959_vcap_props[] = {
[VCAP_ES0] = {
.action_type_width = 0,
.action_table = {
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 12c739cb89f9..4fe51591afa8 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -344,7 +344,7 @@ static const struct ocelot_ops ocelot_ops = {
.netdev_to_port = ocelot_netdev_to_port,
};
-static struct ocelot_vcap_props vsc7514_vcap_props[] = {
+static const struct ocelot_vcap_props vsc7514_vcap_props[] = {
[VCAP_ES0] = {
.action_type_width = 0,
.action_table = {
@@ -638,7 +638,8 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
ocelot->num_phys_ports = of_get_child_count(ports);
ocelot->num_flooding_pgids = 1;
- ocelot->vcap = vsc7514_vcap_props;
+ memcpy(&ocelot->vcap, &vsc7514_vcap_props,
+ OCELOT_NUM_VCAP_BLOCKS * sizeof(*vsc7514_vcap_props));
ocelot->vcap_pol.base = VSC7514_VCAP_POLICER_BASE;
ocelot->vcap_pol.max = VSC7514_VCAP_POLICER_MAX;
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 42634183d062..b097b97993b0 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -525,6 +525,15 @@ enum {
VCAP_CONST_IF_CNT,
};
+enum {
+ VCAP_ES0,
+ VCAP_IS1,
+ VCAP_IS2,
+ __VCAP_COUNT,
+};
+
+#define OCELOT_NUM_VCAP_BLOCKS __VCAP_COUNT
+
enum ocelot_ptp_pins {
PTP_PIN_0,
PTP_PIN_1,
@@ -583,6 +592,29 @@ struct ocelot_vcap_block {
int count;
};
+struct ocelot_vcap_props {
+ u16 tg_width; /* Type-group width (in bits) */
+ u16 sw_count; /* Sub word count */
+ u16 entry_count; /* Entry count */
+ u16 entry_words; /* Number of entry words */
+ u16 entry_width; /* Entry width (in bits) */
+ u16 action_count; /* Action count */
+ u16 action_words; /* Number of action words */
+ u16 action_width; /* Action width (in bits) */
+ u16 action_type_width; /* Action type width (in bits) */
+ struct {
+ u16 width; /* Action type width (in bits) */
+ u16 count; /* Action type sub word count */
+ } action_table[2];
+ u16 counter_words; /* Number of counter words */
+ u16 counter_width; /* Counter width (in bits) */
+
+ enum ocelot_target target;
+
+ const struct vcap_field *keys;
+ const struct vcap_field *actions;
+};
+
struct ocelot_bridge_vlan {
u16 vid;
unsigned long portmask;
@@ -727,7 +759,7 @@ struct ocelot {
struct list_head dummy_rules;
struct ocelot_vcap_block block[3];
struct ocelot_vcap_policer vcap_pol;
- struct ocelot_vcap_props *vcap;
+ struct ocelot_vcap_props vcap[OCELOT_NUM_VCAP_BLOCKS];
struct ocelot_mirror *mirror;
struct ocelot_psfp_list psfp;
diff --git a/include/soc/mscc/ocelot_vcap.h b/include/soc/mscc/ocelot_vcap.h
index 05bd73c63675..96ca1498f722 100644
--- a/include/soc/mscc/ocelot_vcap.h
+++ b/include/soc/mscc/ocelot_vcap.h
@@ -27,38 +27,6 @@
* =================================================================
*/
-enum {
- VCAP_ES0,
- VCAP_IS1,
- VCAP_IS2,
- __VCAP_COUNT,
-};
-
-#define OCELOT_NUM_VCAP_BLOCKS __VCAP_COUNT
-
-struct ocelot_vcap_props {
- u16 tg_width; /* Type-group width (in bits) */
- u16 sw_count; /* Sub word count */
- u16 entry_count; /* Entry count */
- u16 entry_words; /* Number of entry words */
- u16 entry_width; /* Entry width (in bits) */
- u16 action_count; /* Action count */
- u16 action_words; /* Number of action words */
- u16 action_width; /* Action width (in bits) */
- u16 action_type_width; /* Action type width (in bits) */
- struct {
- u16 width; /* Action type width (in bits) */
- u16 count; /* Action type sub word count */
- } action_table[2];
- u16 counter_words; /* Number of counter words */
- u16 counter_width; /* Counter width (in bits) */
-
- enum ocelot_target target;
-
- const struct vcap_field *keys;
- const struct vcap_field *actions;
-};
-
/* VCAP Type-Group values */
#define VCAP_TG_NONE 0 /* Entry is invalid */
#define VCAP_TG_FULL 1 /* Full entry */
--
2.25.1
Hi Colin,
On Fri, Apr 29, 2022 at 04:30:49PM -0700, Colin Foster wrote:
> Each instance of an ocelot struct has the ocelot_vcap_props structure being
> referenced. During initialization (ocelot_init), these vcap_props are
> detected and the structure contents are modified.
>
> In the case of the standard ocelot driver, there will probably only be one
> instance of struct ocelot, since it is part of the chip.
>
> For the Felix driver, there could be multiple instances of struct ocelot.
> In that scenario, the second time ocelot_init would get called, it would
> corrupt what had been done in the first call because they both reference
> *ocelot->vcap. Both of these instances were assigned the same memory
> location.
>
> Move this vcap_props memory to within struct ocelot, so that each instance
> can modify the structure to their heart's content without corrupting other
> instances.
>
> Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
> constants")
>
> Signed-off-by: Colin Foster <[email protected]>
> ---
To prove an issue, you must come with an example of two switches which
share the same struct vcap_props, but contain different VCAP constants
in the hardware registers. Otherwise, what you call "corruption" is just
"overwriting with the same values".
I would say that by definition, if two such switches have different VCAP
constants, they have different vcap_props structures, and if they have
the same vcap_props structure, they have the same VCAP constants.
Therefore, even in a multi-switch environment, a second call to
ocelot_vcap_detect_constants() would overwrite the vcap->entry_width,
vcap->tg_width, vcap->sw_count, vcap->entry_count, vcap->action_count,
vcap->action_width, vcap->counter_words, vcap->counter_width with the
exact same values.
I do not see the point in duplicating struct vcap_props per ocelot
instance.
I assume you are noticing some problems with VSC7512? What are they?
Note that since VSC7512 isn't currently supported by the kernel, even a
theoretical corruption issue doesn't qualify as a bug, since there is no
way to reproduce it. All the Microchip switches supported by the kernel
are internal to an SoC, are single switches, and they have different
vcap_props structures.
Hi Vladimir,
On Sat, Apr 30, 2022 at 02:24:57PM +0000, Vladimir Oltean wrote:
> Hi Colin,
>
> On Fri, Apr 29, 2022 at 04:30:49PM -0700, Colin Foster wrote:
> > Each instance of an ocelot struct has the ocelot_vcap_props structure being
> > referenced. During initialization (ocelot_init), these vcap_props are
> > detected and the structure contents are modified.
> >
> > In the case of the standard ocelot driver, there will probably only be one
> > instance of struct ocelot, since it is part of the chip.
> >
> > For the Felix driver, there could be multiple instances of struct ocelot.
> > In that scenario, the second time ocelot_init would get called, it would
> > corrupt what had been done in the first call because they both reference
> > *ocelot->vcap. Both of these instances were assigned the same memory
> > location.
> >
> > Move this vcap_props memory to within struct ocelot, so that each instance
> > can modify the structure to their heart's content without corrupting other
> > instances.
> >
> > Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
> > constants")
> >
> > Signed-off-by: Colin Foster <[email protected]>
> > ---
>
> To prove an issue, you must come with an example of two switches which
> share the same struct vcap_props, but contain different VCAP constants
> in the hardware registers. Otherwise, what you call "corruption" is just
> "overwriting with the same values".
>
> I would say that by definition, if two such switches have different VCAP
> constants, they have different vcap_props structures, and if they have
> the same vcap_props structure, they have the same VCAP constants.
>
> Therefore, even in a multi-switch environment, a second call to
> ocelot_vcap_detect_constants() would overwrite the vcap->entry_width,
> vcap->tg_width, vcap->sw_count, vcap->entry_count, vcap->action_count,
> vcap->action_width, vcap->counter_words, vcap->counter_width with the
> exact same values.
>
> I do not see the point in duplicating struct vcap_props per ocelot
> instance.
>
> I assume you are noticing some problems with VSC7512? What are they?
I'm not seeing issues, no. I was looking to implement the shared
ocelot_vcap struct between the 7514 and (in-development 7512. In doing
so I came across this realization that these per-file structures could
be referenced multiple times, which was the point of this patch. If the
structure were simply a const configuration there would be no issue, but
since it is half const and half runtime populated it got more complicated.
(that is likely why I didn't make it shared initially... which feels
like ages ago at this point)
Whether or not hardware exists that could be affected by this corner
case I don't know.
> Note that since VSC7512 isn't currently supported by the kernel, even a
> theoretical corruption issue doesn't qualify as a bug, since there is no
> way to reproduce it. All the Microchip switches supported by the kernel
> are internal to an SoC, are single switches, and they have different
> vcap_props structures.
I see. So I do have a misunderstanding in the process.
I shouldn't have submitted this to net, because it isn't an actual "bug"
I observed. Instead it was a potential issue with existing code, and
could have affected certain hardware configurations. How should I have
sent this out? (RFC? net-next? separate conversation discussing the
validity?)
Back to this patch in particular:
You're saying there's no need to duplicate the vcap_props structure
array per ocelot instance. Understood. Would it be an improvement to
split up vcap into a const configuration section (one per hardware
layout) and a detected set? Or would you have any other suggestion?
And, of course, I can drag this along with my 7512 patch set for now, or
try to get this in now. This one feels like it is worth keeping
separate...
And thanks as always for your feedback!
On Fri, 29 Apr 2022 16:30:49 -0700 Colin Foster wrote:
> Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
> constants")
>
> Signed-off-by: Colin Foster <[email protected]>
Please don't line-wrap tags and please don't add empty lines between
them. Let's give Vladimir a day or two to comment on the merits and
please repost with the tags fixed, thanks!
Hi Jakub,
On Fri, Apr 29, 2022 at 07:06:22PM -0700, Jakub Kicinski wrote:
> On Fri, 29 Apr 2022 16:30:49 -0700 Colin Foster wrote:
> > Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
> > constants")
> >
> > Signed-off-by: Colin Foster <[email protected]>
>
> Please don't line-wrap tags and please don't add empty lines between
> them. Let's give Vladimir a day or two to comment on the merits and
> please repost with the tags fixed, thanks!
Yep. I saw that on patchwork. Oops. I'll fix it up in whatever future
patch comes.
Thanks!
On Sat, Apr 30, 2022 at 10:24:00AM -0700, Colin Foster wrote:
> Hi Vladimir,
>
> On Sat, Apr 30, 2022 at 02:24:57PM +0000, Vladimir Oltean wrote:
> > Hi Colin,
> >
> > On Fri, Apr 29, 2022 at 04:30:49PM -0700, Colin Foster wrote:
> > > Each instance of an ocelot struct has the ocelot_vcap_props structure being
> > > referenced. During initialization (ocelot_init), these vcap_props are
> > > detected and the structure contents are modified.
> > >
> > > In the case of the standard ocelot driver, there will probably only be one
> > > instance of struct ocelot, since it is part of the chip.
> > >
> > > For the Felix driver, there could be multiple instances of struct ocelot.
> > > In that scenario, the second time ocelot_init would get called, it would
> > > corrupt what had been done in the first call because they both reference
> > > *ocelot->vcap. Both of these instances were assigned the same memory
> > > location.
> > >
> > > Move this vcap_props memory to within struct ocelot, so that each instance
> > > can modify the structure to their heart's content without corrupting other
> > > instances.
> > >
> > > Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
> > > constants")
> > >
> > > Signed-off-by: Colin Foster <[email protected]>
> > > ---
> >
> > To prove an issue, you must come with an example of two switches which
> > share the same struct vcap_props, but contain different VCAP constants
> > in the hardware registers. Otherwise, what you call "corruption" is just
> > "overwriting with the same values".
> >
> > I would say that by definition, if two such switches have different VCAP
> > constants, they have different vcap_props structures, and if they have
> > the same vcap_props structure, they have the same VCAP constants.
> >
> > Therefore, even in a multi-switch environment, a second call to
> > ocelot_vcap_detect_constants() would overwrite the vcap->entry_width,
> > vcap->tg_width, vcap->sw_count, vcap->entry_count, vcap->action_count,
> > vcap->action_width, vcap->counter_words, vcap->counter_width with the
> > exact same values.
> >
> > I do not see the point in duplicating struct vcap_props per ocelot
> > instance.
> >
> > I assume you are noticing some problems with VSC7512? What are they?
>
> I'm not seeing issues, no. I was looking to implement the shared
> ocelot_vcap struct between the 7514 and (in-development 7512. In doing
> so I came across this realization that these per-file structures could
> be referenced multiple times, which was the point of this patch. If the
> structure were simply a const configuration there would be no issue, but
> since it is half const and half runtime populated it got more complicated.
>
> (that is likely why I didn't make it shared initially... which feels
> like ages ago at this point)
>
> Whether or not hardware exists that could be affected by this corner
> case I don't know.
VSC7512 documentation at the following link, VCAP constants are laid out
in tables 72-74 starting with page 112:
https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf
VSC7514 documentation at the following link, VCAP constants are laid out
in tables 71-73 starting with page 111:
https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf
As you can see, they are identical. Coincidence? I think not. After all,
they are from the same generation and have the same port count.
So even if the new vsc7512 driver reuses the vsc7514 structure for VCAP
properties, and is instantiated in a system where a vsc7514 switch is
also instantiated, I claim that nothing bad will happen. Are you
claiming otherwise? What is that bad thing, exactly?
>
> > Note that since VSC7512 isn't currently supported by the kernel, even a
> > theoretical corruption issue doesn't qualify as a bug, since there is no
> > way to reproduce it. All the Microchip switches supported by the kernel
> > are internal to an SoC, are single switches, and they have different
> > vcap_props structures.
>
> I see. So I do have a misunderstanding in the process.
>
> I shouldn't have submitted this to net, because it isn't an actual "bug"
> I observed. Instead it was a potential issue with existing code, and
> could have affected certain hardware configurations. How should I have
> sent this out? (RFC? net-next? separate conversation discussing the
> validity?)
I can't answer how you should have sent out this patch, since I don't
yet understand what is gained by making the change.
> Back to this patch in particular:
>
> You're saying there's no need to duplicate the vcap_props structure
> array per ocelot instance. Understood. Would it be an improvement to
> split up vcap into a const configuration section (one per hardware
> layout) and a detected set? Or would you have any other suggestion?
Maybe, although I assume the only reason why you're proposing that is
that you want to then proceed and make the detected properties unique
per switch, which again would increase the memory footprint of the
driver for a reason I am not following.
I suppose there's also the option of leaving code that isn't broken
alone?
> And, of course, I can drag this along with my 7512 patch set for now,
Why?
> or try to get this in now. This one feels like it is worth keeping
> separate...
>
> And thanks as always for your feedback!
On Sat, Apr 30, 2022 at 09:56:52PM +0000, Vladimir Oltean wrote:
> On Sat, Apr 30, 2022 at 10:24:00AM -0700, Colin Foster wrote:
> > Hi Vladimir,
> >
> > On Sat, Apr 30, 2022 at 02:24:57PM +0000, Vladimir Oltean wrote:
> > > Hi Colin,
> > >
> > > On Fri, Apr 29, 2022 at 04:30:49PM -0700, Colin Foster wrote:
> > > > Each instance of an ocelot struct has the ocelot_vcap_props structure being
> > > > referenced. During initialization (ocelot_init), these vcap_props are
> > > > detected and the structure contents are modified.
> > > >
> > > > In the case of the standard ocelot driver, there will probably only be one
> > > > instance of struct ocelot, since it is part of the chip.
> > > >
> > > > For the Felix driver, there could be multiple instances of struct ocelot.
> > > > In that scenario, the second time ocelot_init would get called, it would
> > > > corrupt what had been done in the first call because they both reference
> > > > *ocelot->vcap. Both of these instances were assigned the same memory
> > > > location.
> > > >
> > > > Move this vcap_props memory to within struct ocelot, so that each instance
> > > > can modify the structure to their heart's content without corrupting other
> > > > instances.
> > > >
> > > > Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
> > > > constants")
> > > >
> > > > Signed-off-by: Colin Foster <[email protected]>
> > > > ---
> > >
> > > To prove an issue, you must come with an example of two switches which
> > > share the same struct vcap_props, but contain different VCAP constants
> > > in the hardware registers. Otherwise, what you call "corruption" is just
> > > "overwriting with the same values".
> > >
> > > I would say that by definition, if two such switches have different VCAP
> > > constants, they have different vcap_props structures, and if they have
> > > the same vcap_props structure, they have the same VCAP constants.
> > >
> > > Therefore, even in a multi-switch environment, a second call to
> > > ocelot_vcap_detect_constants() would overwrite the vcap->entry_width,
> > > vcap->tg_width, vcap->sw_count, vcap->entry_count, vcap->action_count,
> > > vcap->action_width, vcap->counter_words, vcap->counter_width with the
> > > exact same values.
> > >
> > > I do not see the point in duplicating struct vcap_props per ocelot
> > > instance.
> > >
> > > I assume you are noticing some problems with VSC7512? What are they?
> >
> > I'm not seeing issues, no. I was looking to implement the shared
> > ocelot_vcap struct between the 7514 and (in-development 7512. In doing
> > so I came across this realization that these per-file structures could
> > be referenced multiple times, which was the point of this patch. If the
> > structure were simply a const configuration there would be no issue, but
> > since it is half const and half runtime populated it got more complicated.
> >
> > (that is likely why I didn't make it shared initially... which feels
> > like ages ago at this point)
> >
> > Whether or not hardware exists that could be affected by this corner
> > case I don't know.
>
> VSC7512 documentation at the following link, VCAP constants are laid out
> in tables 72-74 starting with page 112:
> https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf
>
> VSC7514 documentation at the following link, VCAP constants are laid out
> in tables 71-73 starting with page 111:
> https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf
>
> As you can see, they are identical. Coincidence? I think not. After all,
> they are from the same generation and have the same port count.
> So even if the new vsc7512 driver reuses the vsc7514 structure for VCAP
> properties, and is instantiated in a system where a vsc7514 switch is
> also instantiated, I claim that nothing bad will happen. Are you
> claiming otherwise? What is that bad thing, exactly?
I see your point - I misinterpreted the severity here. I agree at the
end of the day we'll possibly write the same values into a memory
location multiple times, and since there's no supported hardware that
differs there won't be a risk. If, at some point in the future, a
chip comes along with slightly different parameters it could become a
problem, but there's no need to solve a problem now that might never
exist.
Thanks for the feedback. I'll drop this patch, as it isn't necessary.
>
> >
> > > Note that since VSC7512 isn't currently supported by the kernel, even a
> > > theoretical corruption issue doesn't qualify as a bug, since there is no
> > > way to reproduce it. All the Microchip switches supported by the kernel
> > > are internal to an SoC, are single switches, and they have different
> > > vcap_props structures.
> >
> > I see. So I do have a misunderstanding in the process.
> >
> > I shouldn't have submitted this to net, because it isn't an actual "bug"
> > I observed. Instead it was a potential issue with existing code, and
> > could have affected certain hardware configurations. How should I have
> > sent this out? (RFC? net-next? separate conversation discussing the
> > validity?)
>
> I can't answer how you should have sent out this patch, since I don't
> yet understand what is gained by making the change.
>
> > Back to this patch in particular:
> >
> > You're saying there's no need to duplicate the vcap_props structure
> > array per ocelot instance. Understood. Would it be an improvement to
> > split up vcap into a const configuration section (one per hardware
> > layout) and a detected set? Or would you have any other suggestion?
>
> Maybe, although I assume the only reason why you're proposing that is
> that you want to then proceed and make the detected properties unique
> per switch, which again would increase the memory footprint of the
> driver for a reason I am not following.
>
> I suppose there's also the option of leaving code that isn't broken
> alone?
>
> > And, of course, I can drag this along with my 7512 patch set for now,
>
> Why?
>
> > or try to get this in now. This one feels like it is worth keeping
> > separate...
> >
> > And thanks as always for your feedback!