| Message ID | 20230128060308.1549707-2-frank@crawford.emu.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: (it87) Complete handling multi-chip configuration | expand |
On Sat, Jan 28, 2023 at 05:03:02PM +1100, Frank Crawford wrote: > Disabling configuration mode on some chips can result in system > hang-ups and access failures to the Super-IO chip at the > second SIO address. Never exit configuration mode on these > chips to avoid the problem. > > This patch should be applied in conjunction with a previous one to > initialise the second chip for certain mother boards. > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > --- > > v3: > * Correct possible uninitialised pointer issue. > > v2: > * Convert to use feature flag and related macros rather than a separate > field, as suggested in review. > * Reverse sense of flag in superio_exit to simplify feature macro. > > --- > drivers/hwmon/it87.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index a8a6a0ffee82..923a9563be92 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -128,10 +128,12 @@ static inline int superio_enter(int ioreg) > return 0; > } > > -static inline void superio_exit(int ioreg) > +static inline void superio_exit(int ioreg, bool noexit) > { > - outb(0x02, ioreg); > - outb(0x02, ioreg + 1); > + if (!noexit) { > + outb(0x02, ioreg); > + outb(0x02, ioreg + 1); > + } > release_region(ioreg, 2); > } > > @@ -300,6 +302,13 @@ struct it87_devices { > #define FEAT_PWM_FREQ2 BIT(16) /* Separate pwm freq 2 */ > #define FEAT_SIX_TEMP BIT(17) /* Up to 6 temp sensors */ > #define FEAT_VIN3_5V BIT(18) /* VIN3 connected to +5V */ > +/* > + * Disabling configuration mode on some chips can result in system > + * hang-ups and access failures to the Super-IO chip at the > + * second SIO address. Never exit configuration mode on these > + * chips to avoid the problem. > + */ > +#define FEAT_CONF_NOEXIT BIT(28) /* Chip should not exit conf mode */ Feature bits are supposed to be numbered sequentially, not randomly assigned. Please use bit 19. > > static const struct it87_devices it87_devices[] = { > [it87] = { > @@ -493,6 +502,7 @@ static const struct it87_devices it87_devices[] = { > #define has_pwm_freq2(data) ((data)->features & FEAT_PWM_FREQ2) > #define has_six_temp(data) ((data)->features & FEAT_SIX_TEMP) > #define has_vin3_5v(data) ((data)->features & FEAT_VIN3_5V) > +#define has_conf_noexit(data) ((data)->features & FEAT_CONF_NOEXIT) > > struct it87_sio_data { > int sioaddr; > @@ -2404,7 +2414,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, > { > int err; > u16 chip_type; > - const struct it87_devices *config; > + const struct it87_devices *config = NULL; > > err = superio_enter(sioaddr); > if (err) > @@ -2489,6 +2499,8 @@ static int __init it87_find(int sioaddr, unsigned short *address, > goto exit; > } > > + config = &it87_devices[sio_data->type]; > + > superio_select(sioaddr, PME); > if (!(superio_inb(sioaddr, IT87_ACT_REG) & 0x01)) { > pr_info("Device not activated, skipping\n"); > @@ -2508,8 +2520,6 @@ static int __init it87_find(int sioaddr, unsigned short *address, > it87_devices[sio_data->type].suffix, > *address, sio_data->revision); > > - config = &it87_devices[sio_data->type]; > - > /* in7 (VSB or VCCH5V) is always internal on some chips */ > if (has_in7_internal(config)) > sio_data->internal |= BIT(1); > @@ -2827,7 +2837,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, > sio_data->skip_pwm |= dmi_data->skip_pwm; > > exit: > - superio_exit(sioaddr); > + superio_exit(sioaddr, config ? has_conf_noexit(config) : false); Nit: "config && has_conf_noexit(config)" would avoid the conditional code. > return err; > } > > @@ -3213,7 +3223,7 @@ static void it87_resume_sio(struct platform_device *pdev) > reg2c); > } > > - superio_exit(data->sioaddr); > + superio_exit(data->sioaddr, has_conf_noexit(data)); > } > > static int it87_resume(struct device *dev)
On 1/29/23 12:56, Guenter Roeck wrote: > On Sat, Jan 28, 2023 at 05:03:02PM +1100, Frank Crawford wrote: >> Disabling configuration mode on some chips can result in system >> hang-ups and access failures to the Super-IO chip at the >> second SIO address. Never exit configuration mode on these >> chips to avoid the problem. >> >> This patch should be applied in conjunction with a previous one to >> initialise the second chip for certain mother boards. >> >> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> >> --- >> >> v3: >> * Correct possible uninitialised pointer issue. >> >> v2: >> * Convert to use feature flag and related macros rather than a separate >> field, as suggested in review. >> * Reverse sense of flag in superio_exit to simplify feature macro. >> >> --- >> drivers/hwmon/it87.c | 26 ++++++++++++++++++-------- >> 1 file changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c >> index a8a6a0ffee82..923a9563be92 100644 >> --- a/drivers/hwmon/it87.c >> +++ b/drivers/hwmon/it87.c >> @@ -128,10 +128,12 @@ static inline int superio_enter(int ioreg) >> return 0; >> } >> >> -static inline void superio_exit(int ioreg) >> +static inline void superio_exit(int ioreg, bool noexit) >> { >> - outb(0x02, ioreg); >> - outb(0x02, ioreg + 1); >> + if (!noexit) { >> + outb(0x02, ioreg); >> + outb(0x02, ioreg + 1); >> + } >> release_region(ioreg, 2); >> } >> >> @@ -300,6 +302,13 @@ struct it87_devices { >> #define FEAT_PWM_FREQ2 BIT(16) /* Separate pwm freq 2 */ >> #define FEAT_SIX_TEMP BIT(17) /* Up to 6 temp sensors */ >> #define FEAT_VIN3_5V BIT(18) /* VIN3 connected to +5V */ >> +/* >> + * Disabling configuration mode on some chips can result in system >> + * hang-ups and access failures to the Super-IO chip at the >> + * second SIO address. Never exit configuration mode on these >> + * chips to avoid the problem. >> + */ >> +#define FEAT_CONF_NOEXIT BIT(28) /* Chip should not exit conf mode */ > > Feature bits are supposed to be numbered sequentially, not randomly > assigned. Please use bit 19. > Given that the commit window is coming up, and since I don't have other issues with your series, I am inclined to apply it with the bit changed. Please let me know in the next day or two if this is acceptable for you. If not, the series will have to wait until v6.4. Thanks, Guenter
On Mon, 2023-01-30 at 10:42 -0800, Guenter Roeck wrote: > On 1/29/23 12:56, Guenter Roeck wrote: > > On Sat, Jan 28, 2023 at 05:03:02PM +1100, Frank Crawford wrote: > > > Disabling configuration mode on some chips can result in system > > > hang-ups and access failures to the Super-IO chip at the > > > second SIO address. Never exit configuration mode on these > > > chips to avoid the problem. > > > > > > This patch should be applied in conjunction with a previous one > > > to > > > initialise the second chip for certain mother boards. > > > > > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > > > --- > > > > > > v3: > > > * Correct possible uninitialised pointer issue. > > > > > > v2: > > > * Convert to use feature flag and related macros rather than a > > > separate > > > field, as suggested in review. > > > * Reverse sense of flag in superio_exit to simplify feature > > > macro. > > > > > > --- > > > drivers/hwmon/it87.c | 26 ++++++++++++++++++-------- > > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > > > index a8a6a0ffee82..923a9563be92 100644 > > > --- a/drivers/hwmon/it87.c > > > +++ b/drivers/hwmon/it87.c > > > @@ -128,10 +128,12 @@ static inline int superio_enter(int ioreg) > > > return 0; > > > } > > > > > > -static inline void superio_exit(int ioreg) > > > +static inline void superio_exit(int ioreg, bool noexit) > > > { > > > - outb(0x02, ioreg); > > > - outb(0x02, ioreg + 1); > > > + if (!noexit) { > > > + outb(0x02, ioreg); > > > + outb(0x02, ioreg + 1); > > > + } > > > release_region(ioreg, 2); > > > } > > > > > > @@ -300,6 +302,13 @@ struct it87_devices { > > > #define FEAT_PWM_FREQ2 BIT(16) /* Separate pwm > > > freq 2 */ > > > #define FEAT_SIX_TEMP BIT(17) /* Up to 6 temp sensors > > > */ > > > #define FEAT_VIN3_5V BIT(18) /* VIN3 connected to +5V > > > */ > > > +/* > > > + * Disabling configuration mode on some chips can result in > > > system > > > + * hang-ups and access failures to the Super-IO chip at the > > > + * second SIO address. Never exit configuration mode on these > > > + * chips to avoid the problem. > > > + */ > > > +#define FEAT_CONF_NOEXIT BIT(28) /* Chip should not exit > > > conf mode */ > > > > Feature bits are supposed to be numbered sequentially, not randomly > > assigned. Please use bit 19. > > > > Given that the commit window is coming up, and since I don't have > other issues with your series, I am inclined to apply it with the > bit changed. Please let me know in the next day or two if this is > acceptable for you. If not, the series will have to wait until v6.4. > Yes, I'm okay with that. I was planning to submit an update with the only changes being both of your changes in your previous comment, but the second one can wait for a later group if you are happy with that. > Thanks, > Guenter > Regards Frank
On 1/30/23 14:19, Frank Crawford wrote: > On Mon, 2023-01-30 at 10:42 -0800, Guenter Roeck wrote: >> On 1/29/23 12:56, Guenter Roeck wrote: >>> On Sat, Jan 28, 2023 at 05:03:02PM +1100, Frank Crawford wrote: >>>> Disabling configuration mode on some chips can result in system >>>> hang-ups and access failures to the Super-IO chip at the >>>> second SIO address. Never exit configuration mode on these >>>> chips to avoid the problem. >>>> >>>> This patch should be applied in conjunction with a previous one >>>> to >>>> initialise the second chip for certain mother boards. >>>> >>>> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> >>>> --- >>>> >>>> v3: >>>> * Correct possible uninitialised pointer issue. >>>> >>>> v2: >>>> * Convert to use feature flag and related macros rather than a >>>> separate >>>> field, as suggested in review. >>>> * Reverse sense of flag in superio_exit to simplify feature >>>> macro. >>>> >>>> --- >>>> drivers/hwmon/it87.c | 26 ++++++++++++++++++-------- >>>> 1 file changed, 18 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c >>>> index a8a6a0ffee82..923a9563be92 100644 >>>> --- a/drivers/hwmon/it87.c >>>> +++ b/drivers/hwmon/it87.c >>>> @@ -128,10 +128,12 @@ static inline int superio_enter(int ioreg) >>>> return 0; >>>> } >>>> >>>> -static inline void superio_exit(int ioreg) >>>> +static inline void superio_exit(int ioreg, bool noexit) >>>> { >>>> - outb(0x02, ioreg); >>>> - outb(0x02, ioreg + 1); >>>> + if (!noexit) { >>>> + outb(0x02, ioreg); >>>> + outb(0x02, ioreg + 1); >>>> + } >>>> release_region(ioreg, 2); >>>> } >>>> >>>> @@ -300,6 +302,13 @@ struct it87_devices { >>>> #define FEAT_PWM_FREQ2 BIT(16) /* Separate pwm >>>> freq 2 */ >>>> #define FEAT_SIX_TEMP BIT(17) /* Up to 6 temp sensors >>>> */ >>>> #define FEAT_VIN3_5V BIT(18) /* VIN3 connected to +5V >>>> */ >>>> +/* >>>> + * Disabling configuration mode on some chips can result in >>>> system >>>> + * hang-ups and access failures to the Super-IO chip at the >>>> + * second SIO address. Never exit configuration mode on these >>>> + * chips to avoid the problem. >>>> + */ >>>> +#define FEAT_CONF_NOEXIT BIT(28) /* Chip should not exit >>>> conf mode */ >>> >>> Feature bits are supposed to be numbered sequentially, not randomly >>> assigned. Please use bit 19. >>> >> >> Given that the commit window is coming up, and since I don't have >> other issues with your series, I am inclined to apply it with the >> bit changed. Please let me know in the next day or two if this is >> acceptable for you. If not, the series will have to wait until v6.4. >> > > Yes, I'm okay with that. > > I was planning to submit an update with the only changes being both of > your changes in your previous comment, but the second one can wait for > a later group if you are happy with that. > The other change request was a minor optimization. Just leave it alone. Series applied to hwmon-next, with bit changed from 28 to 19. Guenter
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index a8a6a0ffee82..923a9563be92 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -128,10 +128,12 @@ static inline int superio_enter(int ioreg) return 0; } -static inline void superio_exit(int ioreg) +static inline void superio_exit(int ioreg, bool noexit) { - outb(0x02, ioreg); - outb(0x02, ioreg + 1); + if (!noexit) { + outb(0x02, ioreg); + outb(0x02, ioreg + 1); + } release_region(ioreg, 2); } @@ -300,6 +302,13 @@ struct it87_devices { #define FEAT_PWM_FREQ2 BIT(16) /* Separate pwm freq 2 */ #define FEAT_SIX_TEMP BIT(17) /* Up to 6 temp sensors */ #define FEAT_VIN3_5V BIT(18) /* VIN3 connected to +5V */ +/* + * Disabling configuration mode on some chips can result in system + * hang-ups and access failures to the Super-IO chip at the + * second SIO address. Never exit configuration mode on these + * chips to avoid the problem. + */ +#define FEAT_CONF_NOEXIT BIT(28) /* Chip should not exit conf mode */ static const struct it87_devices it87_devices[] = { [it87] = { @@ -493,6 +502,7 @@ static const struct it87_devices it87_devices[] = { #define has_pwm_freq2(data) ((data)->features & FEAT_PWM_FREQ2) #define has_six_temp(data) ((data)->features & FEAT_SIX_TEMP) #define has_vin3_5v(data) ((data)->features & FEAT_VIN3_5V) +#define has_conf_noexit(data) ((data)->features & FEAT_CONF_NOEXIT) struct it87_sio_data { int sioaddr; @@ -2404,7 +2414,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, { int err; u16 chip_type; - const struct it87_devices *config; + const struct it87_devices *config = NULL; err = superio_enter(sioaddr); if (err) @@ -2489,6 +2499,8 @@ static int __init it87_find(int sioaddr, unsigned short *address, goto exit; } + config = &it87_devices[sio_data->type]; + superio_select(sioaddr, PME); if (!(superio_inb(sioaddr, IT87_ACT_REG) & 0x01)) { pr_info("Device not activated, skipping\n"); @@ -2508,8 +2520,6 @@ static int __init it87_find(int sioaddr, unsigned short *address, it87_devices[sio_data->type].suffix, *address, sio_data->revision); - config = &it87_devices[sio_data->type]; - /* in7 (VSB or VCCH5V) is always internal on some chips */ if (has_in7_internal(config)) sio_data->internal |= BIT(1); @@ -2827,7 +2837,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, sio_data->skip_pwm |= dmi_data->skip_pwm; exit: - superio_exit(sioaddr); + superio_exit(sioaddr, config ? has_conf_noexit(config) : false); return err; } @@ -3213,7 +3223,7 @@ static void it87_resume_sio(struct platform_device *pdev) reg2c); } - superio_exit(data->sioaddr); + superio_exit(data->sioaddr, has_conf_noexit(data)); } static int it87_resume(struct device *dev)
Disabling configuration mode on some chips can result in system hang-ups and access failures to the Super-IO chip at the second SIO address. Never exit configuration mode on these chips to avoid the problem. This patch should be applied in conjunction with a previous one to initialise the second chip for certain mother boards. Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> --- v3: * Correct possible uninitialised pointer issue. v2: * Convert to use feature flag and related macros rather than a separate field, as suggested in review. * Reverse sense of flag in superio_exit to simplify feature macro. --- drivers/hwmon/it87.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)