Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables
On 2023-12-17 13:02:01+0100, Joel Granados wrote:
> Catching up with mail
>
> On Tue, Dec 12, 2023 at 11:51:30PM -0800, Luis Chamberlain wrote:
> > On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote:
> > > My idea was to do something similar to your originl RFC, where you have
> > > an temporary proc_handler something like proc_hdlr_const (we would need
> > > to work on the name) and move each subsystem to the new handler while
> > > the others stay with the non-const one. At the end, the old proc_handler
> > > function name would disapear and would be completely replaced by the new
> > > proc_hdlr_const.
> > >
> > > This is of course extra work and might not be worth it if you don't get
> > > negative feedback related to tree-wide changes. Therefore I stick to my
> > > previous suggestion. Send the big tree-wide patches and only explore
> > > this option if someone screams.
> >
> > I think we can do better, can't we just increase confidence in that we
> > don't *need* muttable ctl_cables with something like smatch or
> > coccinelle so that we can just make them const?
> >
> > Seems like a noble endeavor for us to generalize.
> >
> > Then we just breeze through by first fixing those that *are* using
> > mutable tables by having it just de-register and then re-register
> So let me see if I understand your {de,re}-register idea:
> When we have a situation (like in the networking code) where a ctl_table
> is being used in an unmuttable way, we do your {de,re}-register trick.
unmuttable?
> The trick consists in unregistering an old ctl_table and reregistering
> with a whole new const changed table. In this way, whatever we register
> is always const.
>
> Once we address all the places where this happens, then we just change
> the handler to const and we are done.
>
> Is that correct?
I'm confused.
The handlers can already be made const as shown in this series, which
does convert the whole kernel tree.
There is only one handler (the stackleak one) which modifies the table
and this one is fixed as part of the series.
(Plus the changes needed to the sysctl core to avoid mutation there)
> If that is indeed what you are proposing, you might not even need the
> un-register step as all the mutability that I have seen occurs before
> the register. So maybe instead of re-registering it, you can so a copy
> (of the changed ctl_table) to a const pointer and then pass that along
> to the register function.
Tables that are modified, but *not* through the handler, would crop
during the constification of the table structs.
Which should be a second step.
But Luis' message was not completely clear to me.
I guess I'm missing something.
> Can't think of anything else off the top of my head. Would have to
> actually see the code to evaluate further I think.
>
> > new tables if they need to be changed, and then a new series is sent
> > once we fix all those muttable tables.
Thomas
Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables
Catching up with mail
On Tue, Dec 12, 2023 at 11:51:30PM -0800, Luis Chamberlain wrote:
> On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote:
> > My idea was to do something similar to your originl RFC, where you have
> > an temporary proc_handler something like proc_hdlr_const (we would need
> > to work on the name) and move each subsystem to the new handler while
> > the others stay with the non-const one. At the end, the old proc_handler
> > function name would disapear and would be completely replaced by the new
> > proc_hdlr_const.
> >
> > This is of course extra work and might not be worth it if you don't get
> > negative feedback related to tree-wide changes. Therefore I stick to my
> > previous suggestion. Send the big tree-wide patches and only explore
> > this option if someone screams.
>
> I think we can do better, can't we just increase confidence in that we
> don't *need* muttable ctl_cables with something like smatch or
> coccinelle so that we can just make them const?
>
> Seems like a noble endeavor for us to generalize.
>
> Then we just breeze through by first fixing those that *are* using
> mutable tables by having it just de-register and then re-register
So let me see if I understand your {de,re}-register idea:
When we have a situation (like in the networking code) where a ctl_table
is being used in an unmuttable way, we do your {de,re}-register trick.
The trick consists in unregistering an old ctl_table and reregistering
with a whole new const changed table. In this way, whatever we register
is always const.
Once we address all the places where this happens, then we just change
the handler to const and we are done.
Is that correct?
If that is indeed what you are proposing, you might not even need the
un-register step as all the mutability that I have seen occurs before
the register. So maybe instead of re-registering it, you can so a copy
(of the changed ctl_table) to a const pointer and then pass that along
to the register function.
Can't think of anything else off the top of my head. Would have to
actually see the code to evaluate further I think.
> new tables if they need to be changed, and then a new series is sent
> once we fix all those muttable tables.
>
> Luis
best
--
Joel Granados
signature.asc
Description: PGP signature
Re: [PATCH v3] iio: sx9324: avoid copying property strings
On Sun, 2023-12-17 at 13:24 +, Jonathan Cameron wrote: > On Mon, 11 Dec 2023 22:30:12 -0800 > Joe Perches wrote: > > > On Tue, 2023-12-12 at 00:42 +, Justin Stitt wrote: > > > We're doing some needless string copies when trying to assign the proper > > > `prop` string. We can make `prop` a const char* and simply assign to > > > string literals. > > > > trivia: > > > > I would have updated it like this moving the > > various declarations into the case blocks > > where they are used and removing a few unused > > #defines > > I'd definitely like to see those defines gone. > Arguably an unrelated change as I guess they are left from a previous refactor > of this code. > > Why prop to type renaming? random, no specific need, though I prefer not reusing identifiers with different types in separate local scopes. > It's getting passed into calls where the parameter > is propname so I'd understand renaming to that, but type just seems a bit > random > to me. I do wonder if we are better off having some long lines and getting > rid > of the property naming local variables completely by just duplicating > the device_property_read_u32() call and passing them in directly. maybe, give it a try and see what you think.
Re: [PATCH v3] iio: sx9324: avoid copying property strings
On Mon, 11 Dec 2023 22:30:12 -0800
Joe Perches wrote:
> On Tue, 2023-12-12 at 00:42 +, Justin Stitt wrote:
> > We're doing some needless string copies when trying to assign the proper
> > `prop` string. We can make `prop` a const char* and simply assign to
> > string literals.
>
> trivia:
>
> I would have updated it like this moving the
> various declarations into the case blocks
> where they are used and removing a few unused
> #defines
I'd definitely like to see those defines gone.
Arguably an unrelated change as I guess they are left from a previous refactor
of this code.
Why prop to type renaming? It's getting passed into calls where the parameter
is propname so I'd understand renaming to that, but type just seems a bit random
to me. I do wonder if we are better off having some long lines and getting rid
of the property naming local variables completely by just duplicating
the device_property_read_u32() call and passing them in directly.
Moving declarations more locally is a nice to have but I'll leave that up to
Justin.
Anyhow, both solutions look much better than the original so I'm fine either way
(subject to responses to Stephen's review)
>
> ---
> drivers/iio/proximity/sx9324.c | 69
> +-
> 1 file changed, 41 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> index ac2ed2da21ccc..c50c1108a69cc 100644
> --- a/drivers/iio/proximity/sx9324.c
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -877,17 +877,8 @@ static const struct sx_common_reg_default *
> sx9324_get_default_reg(struct device *dev, int idx,
> struct sx_common_reg_default *reg_def)
> {
> - static const char * const sx9324_rints[] = { "lowest", "low", "high",
> - "highest" };
> - static const char * const sx9324_csidle[] = { "hi-z", "hi-z", "gnd",
> - "vdd" };
> -#define SX9324_PIN_DEF "semtech,ph0-pin"
> -#define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
> -#define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
> - unsigned int pin_defs[SX9324_NUM_PINS];
> - char prop[] = SX9324_PROXRAW_DEF;
> - u32 start = 0, raw = 0, pos = 0;
> - int ret, count, ph, pin;
> + u32 raw = 0;
> + int ret;
>
> memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
>
> @@ -896,7 +887,13 @@ sx9324_get_default_reg(struct device *dev, int idx,
> case SX9324_REG_AFE_PH0:
> case SX9324_REG_AFE_PH1:
> case SX9324_REG_AFE_PH2:
> - case SX9324_REG_AFE_PH3:
> + case SX9324_REG_AFE_PH3: {
> + unsigned int pin_defs[SX9324_NUM_PINS];
> + int count;
> + int pin;
> + int ph;
> + char prop[32];
> +
> ph = reg_def->reg - SX9324_REG_AFE_PH0;
> snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
>
> @@ -913,7 +910,15 @@ sx9324_get_default_reg(struct device *dev, int idx,
> SX9324_REG_AFE_PH0_PIN_MASK(pin);
> reg_def->def = raw;
> break;
> - case SX9324_REG_AFE_CTRL0:
> + }
> + case SX9324_REG_AFE_CTRL0: {
> + static const char * const sx9324_csidle[] = {
> + "hi-z", "hi-z", "gnd", "vdd"
> + };
> + static const char * const sx9324_rints[] = {
> + "lowest", "low", "high", "highest"
> + };
> +
> ret = device_property_match_property_string(dev,
> "semtech,cs-idle-sleep",
> sx9324_csidle,
>
> ARRAY_SIZE(sx9324_csidle));
> @@ -930,16 +935,17 @@ sx9324_get_default_reg(struct device *dev, int idx,
> reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
> }
> break;
> + }
> case SX9324_REG_AFE_CTRL4:
> - case SX9324_REG_AFE_CTRL7:
> + case SX9324_REG_AFE_CTRL7: {
> + const char *type;
> +
> if (reg_def->reg == SX9324_REG_AFE_CTRL4)
> - strncpy(prop, "semtech,ph01-resolution",
> - ARRAY_SIZE(prop));
> + type = "semtech,ph01-resolution";
> else
> - strncpy(prop, "semtech,ph23-resolution",
> - ARRAY_SIZE(prop));
> + type = "semtech,ph23-resolution";
>
> - ret = device_property_read_u32(dev, prop, &raw);
> + ret = device_property_read_u32(dev, type, &raw);
> if (ret)
> break;
>
> @@ -949,6 +955,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
> reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
> raw);
> break;
> + }
> case SX9324_REG_AFE_CTRL8:
> r
Re: [PATCH v2] iio: sx9324: avoid copying property strings
On Tue, 12 Dec 2023 15:51:04 -0800
Gwendal Grignou wrote:
> Reviewed-by: Gwendal Grignou
Hi Gwendal
I'll ignore this tag given the email you've replied to says there is a different
implementation. Please take a look at that version instead.
Jonathan
>
> On Mon, Dec 11, 2023 at 4:46 PM Justin Stitt wrote:
> >
> > Hi,
> >
> > On Mon, Oct 30, 2023 at 2:44 PM Stephen Boyd wrote:
> > >
> > >
> > > We need to free it in other places too, like if the count doesn't match.
> > > It may be easier to extract this section and just have 4 string
> > > literals.
> > >
> > > switch (reg_def->reg) {
> > > case SX9324_REG_AFE_PH0:
> > > reg_def = sx9324_parse_phase_prop(dev, reg_def,
> > > "semtech,ph0-pin");
> > > break;
> > > case SX9324_REG_AFE_PH1:
> > > reg_def = sx9324_parse_phase_prop(dev, reg_def,
> > > "semtech,ph1-pin");
> > > break;
> > > case SX9324_REG_AFE_PH2:
> > > reg_def = sx9324_parse_phase_prop(dev, reg_def,
> > > "semtech,ph2-pin");
> > > break;
> > > case SX9324_REG_AFE_PH3:
> > > reg_def = sx9324_parse_phase_prop(dev, reg_def,
> > > "semtech,ph3-pin");
> > > break;
> > >
> >
> > I've submitted v3 of this patch [1] trying out Stephen's idea. I'd
> > appreciate feedback.
> >
> > [1]:
> > https://lore.kernel.org/all/20231212-strncpy-drivers-iio-proximity-sx9324-c-v3-1-b8ae12fc8...@google.com/
> >
> > Thanks
> > Justin
Re: [PATCH v2][wireless-next/for-next] wifi: mt76: mt7996: Use DECLARE_FLEX_ARRAY() and fix -Warray-bounds warnings
"Gustavo A. R. Silva" wrote: > Transform zero-length arrays `rate`, `adm_stat` and `msdu_cnt` into > proper flexible-array members in anonymous union in `struct > mt7996_mcu_all_sta_info_event` via the DECLARE_FLEX_ARRAY() > helper; and fix multiple -Warray-bounds warnings: > > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:544:61: warning: array > subscript is outside array bounds of 'struct [0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:551:58: warning: array > subscript is outside array bounds of 'struct [0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:553:58: warning: array > subscript is outside array bounds of 'struct [0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:530:61: warning: array > subscript is outside array bounds of 'struct [0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:538:66: warning: array > subscript is outside array bounds of 'struct [0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:540:66: warning: array > subscript is outside array bounds of 'struct [0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:520:57: warning: array > subscript is outside array bounds of 'struct all_sta_trx_rate[0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:526:76: warning: array > subscript is outside array bounds of 'struct all_sta_trx_rate[0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:526:76: warning: array > subscript is outside array bounds of 'struct all_sta_trx_rate[0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:526:76: warning: array > subscript is outside array bounds of 'struct all_sta_trx_rate[0]' > [-Warray-bounds=] > drivers/net/wireless/mediatek/mt76/mt7996/mcu.c:526:76: warning: array > subscript is outside array bounds of 'struct all_sta_trx_rate[0]' > [-Warray-bounds=] > > This results in no differences in binary output, helps with the ongoing > efforts to globally enable -Warray-bounds. > > Reviewed-by: Kees Cook > Signed-off-by: Gustavo A. R. Silva Patch applied to wireless-next.git, thanks. 40d51f70f082 wifi: mt76: mt7996: Use DECLARE_FLEX_ARRAY() and fix -Warray-bounds warnings -- https://patchwork.kernel.org/project/linux-wireless/patch/ZXiU9ayVCslt3qiI@work/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
