Re: [alsa-devel] [PATCH] ASoC: fsl_asrc: constify snd_soc_dai_ops structure
Hi Mark, Quoting Mark Brown <broo...@kernel.org>: On Thu, Jul 13, 2017 at 09:32:41AM +0200, Takashi Iwai wrote: please stop posting in this style. It's really annoying to see spontaneously popping-up almost same patch for more than two hours long. If you have a series of the same fix patches, send them as a patch set in a shot with a thread. git-send-email does it right. I don't mind a couple of patches posted separately, but this is over the limit. Or at least just collect them up and send them all at one time even if not as a single thread (you don't want to CC everyone affected by a single patch in the set on everything, that's harder to avoid when sending a series via git, but it can be confusing to get one item in a large patch series without context). I like this idea better. I will do so next time. :) Thank you -- Gustavo A. R. Silva
Re: [alsa-devel] [PATCH] ASoC: fsl_asrc: constify snd_soc_dai_ops structure
Hi Joe, Quoting Joe Perches <j...@perches.com>: On Thu, 2017-07-13 at 10:18 -0500, Gustavo A. R. Silva wrote: Hi Mark, Quoting Mark Brown <broo...@kernel.org>: > On Thu, Jul 13, 2017 at 09:32:41AM +0200, Takashi Iwai wrote: > > > please stop posting in this style. It's really annoying to see > > spontaneously popping-up almost same patch for more than two hours > > long. > > If you have a series of the same fix patches, send them as a patch > > set in a shot with a thread. git-send-email does it right. > > I don't mind a couple of patches posted separately, but this is over > > the limit. > > Or at least just collect them up and send them all at one time even if > not as a single thread (you don't want to CC everyone affected by a > single patch in the set on everything, that's harder to avoid when > sending a series via git, but it can be confusing to get one item in a > large patch series without context). I like this idea better. I will do so next time. :) I don't it's better. It's not that confusing if the 0/n patch cover letter is cc'd to all the appropriate mailing lists and all the [1..n]/n patches are sent with in-reply-to of the cover letter and send to the maintainers and appropriate mailing lists. I ended up following your suggestions: https://lkml.org/lkml/2017/7/13/739 (Notice that these are new patches. Not related to the ones I previously sent) Much appreciated Thanks! -- Gustavo A. R. Silva
[PATCH] usb: gadget: fsl_udc_core: compress return logic into one line
Simplify return logic to avoid unnecessary variable assignment.
This issue was detected using Coccinelle and the following
semantic patch:
@@
local idexpression ret;
expression e;
@@
-ret =
+return
e;
-return ret;
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
drivers/usb/gadget/udc/fsl_udc_core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c
b/drivers/usb/gadget/udc/fsl_udc_core.c
index 6f2f71c..bf8ff5e 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -1642,8 +1642,7 @@ static int process_ep_req(struct fsl_udc *udc, int pipe,
} else if (hc32_to_cpu(curr_td->size_ioc_sts)
& DTD_STATUS_ACTIVE) {
VDBG("Request not complete");
- status = REQ_UNCOMPLETE;
- return status;
+ return REQ_UNCOMPLETE;
} else if (remaining_length) {
if (direction) {
VDBG("Transmit dTD remaining length not zero");
--
2.5.0
[PATCH] ASoC: fsl_esai: constify snd_soc_dai_ops structure
This structure is only stored in the ops field of a snd_soc_dai_driver
structure. That field is declared const, so snd_soc_dai_ops structures
that have this property can be declared as const also.
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
sound/soc/fsl/fsl_esai.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 809a069..cef79a1 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -620,7 +620,7 @@ static int fsl_esai_trigger(struct snd_pcm_substream
*substream, int cmd,
return 0;
}
-static struct snd_soc_dai_ops fsl_esai_dai_ops = {
+static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
.startup = fsl_esai_startup,
.shutdown = fsl_esai_shutdown,
.trigger = fsl_esai_trigger,
--
2.5.0
[PATCH] ASoC: fsl_asrc: constify snd_soc_dai_ops structure
This structure is only stored in the ops field of a snd_soc_dai_driver
structure. That field is declared const, so snd_soc_dai_ops structures
that have this property can be declared as const also.
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
sound/soc/fsl/fsl_asrc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 8cfffa7..806d399 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -542,7 +542,7 @@ static int fsl_asrc_dai_trigger(struct snd_pcm_substream
*substream, int cmd,
return 0;
}
-static struct snd_soc_dai_ops fsl_asrc_dai_ops = {
+static const struct snd_soc_dai_ops fsl_asrc_dai_ops = {
.hw_params= fsl_asrc_dai_hw_params,
.hw_free = fsl_asrc_dai_hw_free,
.trigger = fsl_asrc_dai_trigger,
--
2.5.0
[PATCH] ASoC: fsl_spdif: constify snd_soc_dai_ops structure
This structure is only stored in the ops field of a snd_soc_dai_driver
structure. That field is declared const, so snd_soc_dai_ops structures
that have this property can be declared as const also.
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
sound/soc/fsl/fsl_spdif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
index 1ff467c..7e6cc4d 100644
--- a/sound/soc/fsl/fsl_spdif.c
+++ b/sound/soc/fsl/fsl_spdif.c
@@ -626,7 +626,7 @@ static int fsl_spdif_trigger(struct snd_pcm_substream
*substream,
return 0;
}
-static struct snd_soc_dai_ops fsl_spdif_dai_ops = {
+static const struct snd_soc_dai_ops fsl_spdif_dai_ops = {
.startup = fsl_spdif_startup,
.hw_params = fsl_spdif_hw_params,
.trigger = fsl_spdif_trigger,
--
2.5.0
Re: [alsa-devel] [PATCH] ASoC: fsl_asrc: constify snd_soc_dai_ops structure
Hi Takashi,
Quoting Takashi Iwai <ti...@suse.de>:
Gustavo,
please stop posting in this style. It's really annoying to see
spontaneously popping-up almost same patch for more than two hours
long.
If you have a series of the same fix patches, send them as a patch
set in a shot with a thread. git-send-email does it right.
I will do that.
Thanks for the suggestion.
--
Gustavo A. R. Silva
I don't mind a couple of patches posted separately, but this is over
the limit.
thanks,
Takashi
On Thu, 13 Jul 2017 09:23:51 +0200,
Gustavo A. R. Silva wrote:
This structure is only stored in the ops field of a snd_soc_dai_driver
structure. That field is declared const, so snd_soc_dai_ops structures
that have this property can be declared as const also.
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
sound/soc/fsl/fsl_asrc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 8cfffa7..806d399 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -542,7 +542,7 @@ static int fsl_asrc_dai_trigger(struct
snd_pcm_substream *substream, int cmd,
return 0;
}
-static struct snd_soc_dai_ops fsl_asrc_dai_ops = {
+static const struct snd_soc_dai_ops fsl_asrc_dai_ops = {
.hw_params= fsl_asrc_dai_hw_params,
.hw_free = fsl_asrc_dai_hw_free,
.trigger = fsl_asrc_dai_trigger,
--
2.5.0
___
Alsa-devel mailing list
alsa-de...@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: [alsa-devel] [PATCH] ASoC: fsl_asrc: constify snd_soc_dai_ops structure
Hi Mark, Joe, On 07/14/2017 06:25 AM, Mark Brown wrote: On Fri, Jul 14, 2017 at 04:08:21AM -0700, Joe Perches wrote: On Fri, 2017-07-14 at 12:02 +0100, Mark Brown wrote: On Thu, Jul 13, 2017 at 11:18:11AM -0700, Joe Perches wrote: I don't it's better. It's not that confusing if the 0/n patch cover letter is cc'd to all the appropriate mailing lists and all the [1..n]/n patches are sent with in-reply-to of the cover letter and send to the maintainers and appropriate mailing lists. With large serieses like Gustavo is sending the CC list can easily hit the points where mailing lists start blocking it, and the individual pathces really do need to go to the relevant people so they have sight of them. I agree and that's what I wrote. The set of people who should have sight of the patches is wider than just the maintainers. I'm running get_maintainer.pl in the following way in order to get all the supporters, maintainers and lists: $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback Thank you -- Gustavo A. R. Silva
[PATCH] ASoC: imx-ssi: add check on platform_get_irq return value
Check return value from call to platform_get_irq(),
so in case of failure print error message and propagate
the return value.
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
sound/soc/fsl/imx-ssi.c | 4
1 file changed, 4 insertions(+)
diff --git a/sound/soc/fsl/imx-ssi.c b/sound/soc/fsl/imx-ssi.c
index b95132e..0679061 100644
--- a/sound/soc/fsl/imx-ssi.c
+++ b/sound/soc/fsl/imx-ssi.c
@@ -527,6 +527,10 @@ static int imx_ssi_probe(struct platform_device *pdev)
}
ssi->irq = platform_get_irq(pdev, 0);
+ if (ssi->irq < 0) {
+ dev_err(>dev, "Failed to get IRQ: %d\n", ssi->irq);
+ return ssi->irq;
+ }
ssi->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(ssi->clk)) {
--
2.5.0
Re: macintosh: change some data types from int to bool
Hi Michael, Quoting Michael Ellerman <m...@ellerman.id.au>: "Gustavo A. R. Silva" <garsi...@embeddedor.com> writes: Hi Michael, Quoting Michael Ellerman <patch-notificati...@ellerman.id.au>: On Wed, 2018-01-24 at 01:42:28 UTC, "Gustavo A. R. Silva" wrote: Change the data type of the following variables from int to bool across all macintosh drivers: started slots_started pm121_started wf_smu_started Some of these issues were detected with the help of Coccinelle. Suggested-by: Michael Ellerman <m...@ellerman.id.au> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/4f256d561447c6e1bf8b70e19daae0 cheers Awesome. If I can help out with anything else, please let me know. Sure thing. We have a TODO list of sorts on github, some of them are easy, some are not, feel free to ask here or on an individual issue for help: https://github.com/linuxppc/linux/issues I'm sorry for the late reply. I was addressing some issues on DRM and NET components. I already took a look into the TODO list. I'll ask you about some of the issues on github. Thanks! -- Gustavo
Re: macintosh: change some data types from int to bool
Hi Michael, Quoting Michael Ellerman <patch-notificati...@ellerman.id.au>: On Wed, 2018-01-24 at 01:42:28 UTC, "Gustavo A. R. Silva" wrote: Change the data type of the following variables from int to bool across all macintosh drivers: started slots_started pm121_started wf_smu_started Some of these issues were detected with the help of Coccinelle. Suggested-by: Michael Ellerman <m...@ellerman.id.au> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/4f256d561447c6e1bf8b70e19daae0 cheers Awesome. If I can help out with anything else, please let me know. Thank you -- Gustavo
[PATCH 0/7] ASoC: Mark expected switch fall-throughs
Hi all, In preparation to enabling -Wimplicit-fallthrough, this patchset aims to add some annotations in order to mark switch cases where we are expecting to fall through. Thanks Gustavo A. R. Silva (7): ASoC: davinci-i2s: mark expected switch fall-through ASoC: fsl_esai: Mark expected switch fall-through ASoC: Intel: skl-pcm: Mark expected switch fall-through ASoC: omap-dmic: Mark expected switch fall-throughs ASoC: omap-mcpdm: MArk expected switch fall-throughs ASoC: samsung: i2s: Mark expected switch fall-through ASoC: core: mark expected switch fall-through sound/soc/davinci/davinci-i2s.c | 1 + sound/soc/fsl/fsl_esai.c | 1 + sound/soc/intel/skylake/skl-pcm.c | 1 + sound/soc/omap/omap-dmic.c| 2 ++ sound/soc/omap/omap-mcpdm.c | 4 sound/soc/samsung/i2s.c | 1 + sound/soc/soc-core.c | 1 + 7 files changed, 11 insertions(+) -- 2.7.4
[PATCH 2/7] ASoC: fsl_esai: Mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Addresses-Coverity-ID: 1222121 ("Missing break in switch")
Signed-off-by: Gustavo A. R. Silva
---
sound/soc/fsl/fsl_esai.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 8f43110..c1d1d06 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -249,6 +249,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
break;
case ESAI_HCKT_EXTAL:
ecr |= ESAI_ECR_ETI;
+ /* fall through */
case ESAI_HCKR_EXTAL:
ecr |= ESAI_ECR_ERI;
break;
--
2.7.4
[PATCH] ASoC: fsl_spdif: Use 64-bit arithmetic instead of 32-bit
Add suffix ULL to constant 64 in order to give the compiler complete
information about the proper arithmetic to use.
Notice that such constant is used in a context that expects an
expression of type u64 (64 bits, unsigned) and the following
expression is currently being evaluated using 32-bit arithmetic:
rate[index] * txclk_df * 64
Addresses-Coverity-ID: 1222129 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva
---
sound/soc/fsl/fsl_spdif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
index 9b59d87..740b90d 100644
--- a/sound/soc/fsl/fsl_spdif.c
+++ b/sound/soc/fsl/fsl_spdif.c
@@ -1118,7 +1118,7 @@ static u32 fsl_spdif_txclk_caldiv(struct fsl_spdif_priv
*spdif_priv,
for (sysclk_df = sysclk_dfmin; sysclk_df <= sysclk_dfmax; sysclk_df++) {
for (txclk_df = 1; txclk_df <= 128; txclk_df++) {
- rate_ideal = rate[index] * txclk_df * 64;
+ rate_ideal = rate[index] * txclk_df * 64ULL;
if (round)
rate_actual = clk_round_rate(clk, rate_ideal);
else
--
2.7.4
[PATCH] macintosh/ams-input: Use true and false for boolean values
Assign true or false to boolean variables instead of an integer value. This issue was detected with the help of Coccinelle Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> --- drivers/macintosh/ams/ams-input.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/macintosh/ams/ams-input.c b/drivers/macintosh/ams/ams-input.c index 2edae7d..fe248f6 100644 --- a/drivers/macintosh/ams/ams-input.c +++ b/drivers/macintosh/ams/ams-input.c @@ -91,7 +91,7 @@ static int ams_input_enable(void) return error; } - joystick = 1; + joystick = true; return 0; } @@ -104,7 +104,7 @@ static void ams_input_disable(void) ams_info.idev = NULL; } - joystick = 0; + joystick = false; } static ssize_t ams_input_show_joystick(struct device *dev, -- 2.7.4
Re: [PATCH] drivers/macintosh: Use true for boolean value
Quoting Michael Ellerman <m...@ellerman.id.au>: "Gustavo A. R. Silva" <gust...@embeddedor.com> writes: Assign true or false to boolean variables instead of an integer value. This issue was detected with the help of Coccinelle. Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> --- drivers/macintosh/windfarm_pm72.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) This seems to be common across all those drivers. Can you please send one patch to fix them all to use bool: $ git grep "started = " drivers/macintosh/ drivers/macintosh/therm_adt746x.c: int started = 0; drivers/macintosh/therm_adt746x.c: started = 1; drivers/macintosh/windfarm_pm112.c: slots_started = 1; drivers/macintosh/windfarm_pm112.c: started = 1; drivers/macintosh/windfarm_pm121.c: pm121_started = 1; drivers/macintosh/windfarm_pm72.c: started = 1; drivers/macintosh/windfarm_pm81.c: wf_smu_started = 1; drivers/macintosh/windfarm_pm91.c: wf_smu_started = 1; drivers/macintosh/windfarm_rm31.c: started = 1; cheers Sure, no problem. By the way, I've just found the following similar case: --- a/drivers/macintosh/ams/ams-input.c +++ b/drivers/macintosh/ams/ams-input.c @@ -91,7 +91,7 @@ static int ams_input_enable(void) return error; } - joystick = 1; + joystick = true; return 0; } @@ -104,7 +104,7 @@ static void ams_input_disable(void) ams_info.idev = NULL; } - joystick = 0; + joystick = false; } Do you want me to include them all in the same patch? Thanks -- Gustavo
Re: [PATCH] drivers/macintosh: Use true for boolean value
Quoting "Gustavo A. R. Silva" <garsi...@embeddedor.com>: Quoting Michael Ellerman <m...@ellerman.id.au>: "Gustavo A. R. Silva" <gust...@embeddedor.com> writes: Assign true or false to boolean variables instead of an integer value. This issue was detected with the help of Coccinelle. Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> --- drivers/macintosh/windfarm_pm72.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) This seems to be common across all those drivers. Can you please send one patch to fix them all to use bool: $ git grep "started = " drivers/macintosh/ drivers/macintosh/therm_adt746x.c: int started = 0; drivers/macintosh/therm_adt746x.c: started = 1; drivers/macintosh/windfarm_pm112.c: slots_started = 1; drivers/macintosh/windfarm_pm112.c: started = 1; drivers/macintosh/windfarm_pm121.c: pm121_started = 1; drivers/macintosh/windfarm_pm72.c: started = 1; drivers/macintosh/windfarm_pm81.c: wf_smu_started = 1; drivers/macintosh/windfarm_pm91.c: wf_smu_started = 1; drivers/macintosh/windfarm_rm31.c: started = 1; cheers Sure, no problem. By the way, I've just found the following similar case: --- a/drivers/macintosh/ams/ams-input.c +++ b/drivers/macintosh/ams/ams-input.c @@ -91,7 +91,7 @@ static int ams_input_enable(void) return error; } - joystick = 1; + joystick = true; return 0; } @@ -104,7 +104,7 @@ static void ams_input_disable(void) ams_info.idev = NULL; } - joystick = 0; + joystick = false; } Do you want me to include them all in the same patch? I sent separate patches for this. Thanks -- Gustavo
[PATCH] macintosh: change some data types from int to bool
Change the data type of the following variables from int to bool
across all macintosh drivers:
started
slots_started
pm121_started
wf_smu_started
Some of these issues were detected with the help of Coccinelle.
Suggested-by: Michael Ellerman <m...@ellerman.id.au>
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
drivers/macintosh/therm_adt746x.c | 4 ++--
drivers/macintosh/windfarm_pm112.c | 8
drivers/macintosh/windfarm_pm121.c | 5 +++--
drivers/macintosh/windfarm_pm72.c | 2 +-
drivers/macintosh/windfarm_pm81.c | 5 +++--
drivers/macintosh/windfarm_pm91.c | 5 +++--
drivers/macintosh/windfarm_rm31.c | 2 +-
7 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/macintosh/therm_adt746x.c
b/drivers/macintosh/therm_adt746x.c
index f433521..d7cd5af 100644
--- a/drivers/macintosh/therm_adt746x.c
+++ b/drivers/macintosh/therm_adt746x.c
@@ -230,7 +230,7 @@ static void update_fans_speed (struct thermostat *th)
/* we don't care about local sensor, so we start at sensor 1 */
for (i = 1; i < 3; i++) {
- int started = 0;
+ bool started = false;
int fan_number = (th->type == ADT7460 && i == 2);
int var = th->temps[i] - th->limits[i];
@@ -243,7 +243,7 @@ static void update_fans_speed (struct thermostat *th)
if (abs(var - th->last_var[fan_number]) < 2)
continue;
- started = 1;
+ started = true;
new_speed = fan_speed + ((var-1)*step);
if (new_speed < fan_speed)
diff --git a/drivers/macintosh/windfarm_pm112.c
b/drivers/macintosh/windfarm_pm112.c
index 96d16fc..fec91db 100644
--- a/drivers/macintosh/windfarm_pm112.c
+++ b/drivers/macintosh/windfarm_pm112.c
@@ -96,14 +96,14 @@ static int cpu_last_target;
static struct wf_pid_state backside_pid;
static int backside_tick;
static struct wf_pid_state slots_pid;
-static int slots_started;
+static bool slots_started;
static struct wf_pid_state drive_bay_pid;
static int drive_bay_tick;
static int nr_cores;
static int have_all_controls;
static int have_all_sensors;
-static int started;
+static bool started;
static int failure_state;
#define FAILURE_SENSOR 1
@@ -462,7 +462,7 @@ static void slots_fan_tick(void)
/* first time; initialize things */
printk(KERN_INFO "windfarm: Slots control loop started.\n");
wf_pid_init(_pid, _param);
- slots_started = 1;
+ slots_started = true;
}
err = slots_power->ops->get_value(slots_power, );
@@ -506,7 +506,7 @@ static void pm112_tick(void)
int i, last_failure;
if (!started) {
- started = 1;
+ started = true;
printk(KERN_INFO "windfarm: CPUs control loops started.\n");
for (i = 0; i < nr_cores; ++i) {
if (create_cpu_loop(i) < 0) {
diff --git a/drivers/macintosh/windfarm_pm121.c
b/drivers/macintosh/windfarm_pm121.c
index b350fb8..4d72d8f 100644
--- a/drivers/macintosh/windfarm_pm121.c
+++ b/drivers/macintosh/windfarm_pm121.c
@@ -246,7 +246,8 @@ enum {
static struct wf_control *controls[N_CONTROLS] = {};
/* Set to kick the control loop into life */
-static int pm121_all_controls_ok, pm121_all_sensors_ok, pm121_started;
+static int pm121_all_controls_ok, pm121_all_sensors_ok;
+static bool pm121_started;
enum {
FAILURE_FAN = 1 << 0,
@@ -806,7 +807,7 @@ static void pm121_tick(void)
pm121_create_sys_fans(i);
pm121_create_cpu_fans();
- pm121_started = 1;
+ pm121_started = true;
}
/* skipping ticks */
diff --git a/drivers/macintosh/windfarm_pm72.c
b/drivers/macintosh/windfarm_pm72.c
index e88cfb3..8330215 100644
--- a/drivers/macintosh/windfarm_pm72.c
+++ b/drivers/macintosh/windfarm_pm72.c
@@ -611,7 +611,7 @@ static void pm72_tick(void)
int i, last_failure;
if (!started) {
- started = 1;
+ started = true;
printk(KERN_INFO "windfarm: CPUs control loops started.\n");
for (i = 0; i < nr_chips; ++i) {
if (cpu_setup_pid(i) < 0) {
diff --git a/drivers/macintosh/windfarm_pm81.c
b/drivers/macintosh/windfarm_pm81.c
index 93faf29..d9ea455 100644
--- a/drivers/macintosh/windfarm_pm81.c
+++ b/drivers/macintosh/windfarm_pm81.c
@@ -140,7 +140,8 @@ static struct wf_control *fan_system;
static struct wf_control *cpufreq_clamp;
/* Set to kick the control loop into life */
-static int wf_smu_all_controls_ok, wf_smu_all_sensors_ok, wf_smu_started;
+static int wf_smu_all_controls_ok, wf_smu_all_sensors_ok;
+static bool wf_smu_started;
/* Failure handling.. could be
[PATCH] drivers/macintosh: Use true for boolean value
Assign true or false to boolean variables instead of an integer value.
This issue was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
---
drivers/macintosh/windfarm_pm72.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/macintosh/windfarm_pm72.c
b/drivers/macintosh/windfarm_pm72.c
index e88cfb3..8330215 100644
--- a/drivers/macintosh/windfarm_pm72.c
+++ b/drivers/macintosh/windfarm_pm72.c
@@ -611,7 +611,7 @@ static void pm72_tick(void)
int i, last_failure;
if (!started) {
- started = 1;
+ started = true;
printk(KERN_INFO "windfarm: CPUs control loops started.\n");
for (i = 0; i < nr_chips; ++i) {
if (cpu_setup_pid(i) < 0) {
--
2.7.4
Re: [PATCH 4/4] powerpc: Add -Wimplicit-fallthrough to arch CFLAGS
Hi Michael, On 10/11/18 2:32 AM, Michael Ellerman wrote: > It still pops a few errors, including in linux/signal.h & compat.h, so > it's somewhat aspirational until we can get those fixed up :) > I wonder if you have a log containing those warnings that you can share with me. I'd like to fix them up. Thanks -- Gustavo
Re: [PATCH 4/4] powerpc: Add -Wimplicit-fallthrough to arch CFLAGS
On 10/13/18 3:23 AM, Kees Cook wrote: >> >> $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback >> include/linux/compat.h >> linux-ker...@vger.kernel.org (open list) > > Normally things like that go through akpm, but I'm happy to carry them > if needed. > Oh okay. Let me try through apkm first. :) Thanks -- Gustavo
Re: [PATCH 4/4] powerpc: Add -Wimplicit-fallthrough to arch CFLAGS
On 10/12/18 11:32 AM, Michael Ellerman wrote: > > Sure. The kbuild report up thread has some or most of them. > > But here's a full list: > > Failed 279/290 > http://kisskb.ellerman.id.au/kisskb/head/1d59e2c78793d8aea9949ca71323c4583c78f488/ > Failed: powerpc-next/ppc64_defconfig/powerpc-gcc8 > (http://kisskb.ellerman.id.au/kisskb/buildresult/13542791/log/) > /kisskb/src/arch/powerpc/platforms/powermac/feature.c:1477:6: error: this > statement may fall through [-Werror=implicit-fallthrough=] > /kisskb/src/include/linux/signal.h:180:22: error: this statement may fall > through [-Werror=implicit-fallthrough=] > /kisskb/src/include/linux/compat.h:495:51: error: this statement may fall > through [-Werror=implicit-fallthrough=] > /kisskb/src/include/linux/compat.h:496:51: error: this statement may fall > through [-Werror=implicit-fallthrough=] > /kisskb/src/include/linux/compat.h:494:51: error: this statement may fall > through [-Werror=implicit-fallthrough=] > Failed: powerpc-next/corenet64_smp_defconfig/powerpc-gcc8 > (http://kisskb.ellerman.id.au/kisskb/buildresult/13542786/log/) > /kisskb/src/include/linux/compat.h:494:51: error: this statement may fall > through [-Werror=implicit-fallthrough=] > /kisskb/src/include/linux/compat.h:495:51: error: this statement may fall > through [-Werror=implicit-fallthrough=] > /kisskb/src/include/linux/compat.h:496:51: error: this statement may fall > through [-Werror=implicit-fallthrough=] > Failed: powerpc-next/ppc64le_defconfig/powerpc-gcc8 > (http://kisskb.ellerman.id.au/kisskb/buildresult/13542777/log/) > /kisskb/src/include/linux/signal.h:180:22: error: this statement may fall > through [-Werror=implicit-fallthrough=] > Failed: powerpc-next/powernv_defconfig/powerpc-gcc8 > (http://kisskb.ellerman.id.au/kisskb/buildresult/13542776/log/) > /kisskb/src/include/linux/signal.h:180:22: error: this statement may fall > through [-Werror=implicit-fallthrough=] > Thanks, Michael. Kees, can you take the patches? Apparently, neither signal.h nor compat.h have a dedicated maintainer: $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback include/linux/signal.h linux-ker...@vger.kernel.org (open list) $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback include/linux/compat.h linux-ker...@vger.kernel.org (open list) Thanks -- Gustavo
[PATCH] powerpc/xmon/ppc-opc: Use ARRAY_SIZE macro
Use ARRAY_SIZE instead of dividing sizeof array with sizeof an element.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/xmon/ppc-opc.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/xmon/ppc-opc.c b/arch/powerpc/xmon/ppc-opc.c
index ac2b55b..f3f57a1 100644
--- a/arch/powerpc/xmon/ppc-opc.c
+++ b/arch/powerpc/xmon/ppc-opc.c
@@ -966,8 +966,7 @@ const struct powerpc_operand powerpc_operands[] =
{ 0xff, 11, NULL, NULL, PPC_OPERAND_SIGNOPT },
};
-const unsigned int num_powerpc_operands = (sizeof (powerpc_operands)
- / sizeof (powerpc_operands[0]));
+const unsigned int num_powerpc_operands = ARRAY_SIZE(powerpc_operands);
/* The functions used to insert and extract complicated operands. */
@@ -6980,8 +6979,7 @@ const struct powerpc_opcode powerpc_opcodes[] = {
{"fcfidu.",XRC(63,974,1), XRA_MASK, POWER7|PPCA2, PPCVLE, {FRT,
FRB}},
};
-const int powerpc_num_opcodes =
- sizeof (powerpc_opcodes) / sizeof (powerpc_opcodes[0]);
+const int powerpc_num_opcodes = ARRAY_SIZE(powerpc_opcodes);
/* The VLE opcode table.
@@ -7219,8 +7217,7 @@ const struct powerpc_opcode vle_opcodes[] = {
{"se_bl", BD8(58,0,1),BD8_MASK, PPCVLE, 0, {B8}},
};
-const int vle_num_opcodes =
- sizeof (vle_opcodes) / sizeof (vle_opcodes[0]);
+const int vle_num_opcodes = ARRAY_SIZE(vle_opcodes);
/* The macro table. This is only used by the assembler. */
@@ -7288,5 +7285,4 @@ const struct powerpc_macro powerpc_macros[] = {
{"e_clrlslwi",4, PPCVLE, "e_rlwinm %0,%1,%3,(%2)-(%3),31-(%3)"},
};
-const int powerpc_num_macros =
- sizeof (powerpc_macros) / sizeof (powerpc_macros[0]);
+const int powerpc_num_macros = ARRAY_SIZE(powerpc_macros);
--
2.7.4
Re: [PATCH 4/4] powerpc: Add -Wimplicit-fallthrough to arch CFLAGS
On 10/11/18 3:35 AM, Kees Cook wrote: > On Wed, Oct 10, 2018 at 5:32 PM, Michael Ellerman wrote: >> Kees Cook writes: >>> On Tue, Oct 9, 2018 at 10:13 PM, Michael Ellerman >>> wrote: Warn whenever a switch statement has a fallthrough without a comment annotating it. Signed-off-by: Michael Ellerman >>> >>> Yes please. :) >>> >>> Reviewed-by: Kees Cook >> >> Haha, thanks. >> >> It still pops a few errors, including in linux/signal.h & compat.h, so >> it's somewhat aspirational until we can get those fixed up :) > > Gustavo, any chance you can target those two files? It could get us a > whole architecture using the flag. :) > Yep. Sure thing. I'm already taking a look. Thanks -- Gustavo
Re: [PATCH] powerpc/ps3: Use struct_size() in kzalloc()
Hi Geoff,
On 1/16/19 11:21 AM, Geoff Levand wrote:
Hi Gustavo,
On 1/8/19 1:00 PM, Gustavo A. R. Silva wrote:
One of the more common cases of allocation size calculations is finding the
size of a structure that has a zero-sized array at the end, along with memory
for some number of elements for that array. For example:
struct foo {
int stuff;
void *entry[];
};
instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can now
use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/platforms/ps3/device-init.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/ps3/device-init.c
b/arch/powerpc/platforms/ps3/device-init.c
index e7075aaff1bb..59587b75493d 100644
--- a/arch/powerpc/platforms/ps3/device-init.c
+++ b/arch/powerpc/platforms/ps3/device-init.c
@@ -354,9 +354,7 @@ static int ps3_setup_storage_dev(const struct
ps3_repository_device *repo,
repo->dev_index, repo->dev_type, port, blk_size, num_blocks,
num_regions);
- p = kzalloc(sizeof(struct ps3_storage_device) +
- num_regions * sizeof(struct ps3_storage_region),
- GFP_KERNEL);
+ p = kzalloc(struct_size(p, regions, num_regions), GFP_KERNEL);
if (!p) {
result = -ENOMEM;
goto fail_malloc;
It seems to me the motivation for this change is just to have a
code change. As I've said for other similar patches, I'm reluctant
to accept such trivial changes to the PS3 code because it makes
it harder for me to maintain the code in the long term. When I
need to do a git bisect to track down a problem I generally have
a set of debugging patches that I apply on top of the bisect. A
change to the code like this creates the potential for a patch
conflict that I then need to manually edit to resolve.
This patch is part of a treewide effort aimed at preventing potential
integer overflows during allocation. As the commit log says, the
intention is to promote the use of struct_size instead of an
open-coded form that can be prone to mistakes. This macro is a
defense-in-depth strategy against overflows and it is a good idea
to use it as widely as possible.
I'm not stopping to see how old the code is. I'm only focusing on the
particular context of the memory allocation to understand what is the
name of the zero-sized array that should be used for struct_size(),
and if this macro can accurately replace the open-coded form.
If this change was for relatively new code, or better, for a patch
that was submitted but not yet merged, then my feelings would be
different. I think it would be really useful to have something
that scans patches in patchwork.
I understand your point.
Thanks
--
Gustavo
Re: New linux-next KCFLAGS
On 11/26/18 8:00 AM, Stephen Rothwell wrote: Hi all, On Mon, 26 Nov 2018 17:49:26 +1100 Stephen Rothwell wrote: On Sun, 25 Nov 2018 21:46:20 -0800 Kees Cook wrote: Excellent! (Though, wait, does this mean everyone _else_ will see this too? I'm worried that will be way too noisy...) Ah, yes, you may be right. everyone will see them :-( I may have to figure out another way to do this for tomorrow. OK, starting tomorrow (later today), I have just added -Wimplicit-fallthrough to KCFLAGS in my build scripts. Awesome! Thank you, Stephen. :) -- Gustavo
[PATCH] powerpc/ps3: Use struct_size() in kzalloc()
One of the more common cases of allocation size calculations is finding the
size of a structure that has a zero-sized array at the end, along with memory
for some number of elements for that array. For example:
struct foo {
int stuff;
void *entry[];
};
instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can now
use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/platforms/ps3/device-init.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/ps3/device-init.c
b/arch/powerpc/platforms/ps3/device-init.c
index e7075aaff1bb..59587b75493d 100644
--- a/arch/powerpc/platforms/ps3/device-init.c
+++ b/arch/powerpc/platforms/ps3/device-init.c
@@ -354,9 +354,7 @@ static int ps3_setup_storage_dev(const struct
ps3_repository_device *repo,
repo->dev_index, repo->dev_type, port, blk_size, num_blocks,
num_regions);
- p = kzalloc(sizeof(struct ps3_storage_device) +
- num_regions * sizeof(struct ps3_storage_region),
- GFP_KERNEL);
+ p = kzalloc(struct_size(p, regions, num_regions), GFP_KERNEL);
if (!p) {
result = -ENOMEM;
goto fail_malloc;
--
2.20.1
[PATCH] powerpc/spufs: use struct_size() in kmalloc()
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct foo {
int stuff;
void *entry[];
};
instance = kmalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = kmalloc(struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/platforms/cell/spufs/file.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/file.c
b/arch/powerpc/platforms/cell/spufs/file.c
index ae8123edddc6..48c2477e7e2a 100644
--- a/arch/powerpc/platforms/cell/spufs/file.c
+++ b/arch/powerpc/platforms/cell/spufs/file.c
@@ -2338,9 +2338,8 @@ static int spufs_switch_log_open(struct inode *inode,
struct file *file)
goto out;
}
- ctx->switch_log = kmalloc(sizeof(struct switch_log) +
- SWITCH_LOG_BUFSIZE * sizeof(struct switch_log_entry),
- GFP_KERNEL);
+ ctx->switch_log = kmalloc(struct_size(ctx->switch_log, log,
+ SWITCH_LOG_BUFSIZE), GFP_KERNEL);
if (!ctx->switch_log) {
rc = -ENOMEM;
--
2.20.1
Re: [PATCH V1] ASoC: fsl_esai: replace fall-through with break
On 4/8/19 4:28 AM, S.j. Wang wrote:
> case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
> each other, so replace fall-through with break.
>
If this is correct, then you should use the following "Fixes" tag instead,
which is the one that introduced the bug:
Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch fall-through")
>
because this didn't change any functionality.
> Signed-off-by: Shengjiu Wang
> ---
> sound/soc/fsl/fsl_esai.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index c7410bbfd2af..bad0dfed6b68 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
> *dai, int clk_id,
> break;
> case ESAI_HCKT_EXTAL:
> ecr |= ESAI_ECR_ETI;
Also, you should use a simple assignment operator "=" instead of "|=" in both
cases.
> - /* fall through */
> + break;
> case ESAI_HCKR_EXTAL:
> ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
> break;
>
Thanks
--
Gustavo
[PATCH] tty: hvc_xen: Mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. This patch fixes the following warning: drivers/tty/hvc/hvc_xen.c: In function ‘xencons_backend_changed’: drivers/tty/hvc/hvc_xen.c:493:6: warning: this statement may fall through [-Wimplicit-fallthrough=] if (dev->state == XenbusStateClosed) ^ drivers/tty/hvc/hvc_xen.c:496:2: note: here case XenbusStateClosing: ^~~~ Warning level 3 was used: -Wimplicit-fallthrough=3 Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find. This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough Signed-off-by: Gustavo A. R. Silva --- drivers/tty/hvc/hvc_xen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c index dc43fa96c3de..5ef08905fe05 100644 --- a/drivers/tty/hvc/hvc_xen.c +++ b/drivers/tty/hvc/hvc_xen.c @@ -492,7 +492,7 @@ static void xencons_backend_changed(struct xenbus_device *dev, case XenbusStateClosed: if (dev->state == XenbusStateClosed) break; - /* Missed the backend's CLOSING state -- fallthrough */ + /* fall through - Missed the backend's CLOSING state. */ case XenbusStateClosing: xenbus_frontend_closed(dev); break; -- 2.20.1
Re: [PATCH] powerpc/ptrace: Mitigate potential Spectre v1
Hi Breno,
On 1/29/19 10:38 AM, Breno Leitao wrote:
> Hi Gustavo,
>
> On 1/24/19 3:25 PM, Gustavo A. R. Silva wrote:
>>
>>
>> On 1/24/19 8:01 AM, Breno Leitao wrote:
>>> 'regno' is directly controlled by user space, hence leading to a potential
>>> exploitation of the Spectre variant 1 vulnerability.
>>>
>>> On PTRACE_SETREGS and PTRACE_GETREGS requests, user space passes the
>>> register number that would be read or written. This register number is
>>> called 'regno' which is part of the 'addr' syscall parameter.
>>>
>>> This 'regno' value is checked against the maximum pt_regs structure size,
>>> and then used to dereference it, which matches the initial part of a
>>> Spectre v1 (and Spectre v1.1) attack. The dereferenced value, then,
>>> is returned to userspace in the GETREGS case.
>>>
>>
>> Was this reported by any tool?
>
> It was not. I was writing an userspace tool, which required me to read the
> Ptrace subsystem carefully, then, I just found this case.
>
Great. Good catch.
>> If so, it might be worth mentioning it.
>>
>>> This patch sanitizes 'regno' before using it to dereference pt_reg.
>>>
>>> Notice that given that speculation windows are large, the policy is
>>> to kill the speculation on the first load and not worry if it can be
>>> completed with a dependent load/store [1].
>>>
>>> [1] https://marc.info/?l=linux-kernel=152449131114778=2
>>>
>>> Signed-off-by: Breno Leitao
>>> ---
>>> arch/powerpc/kernel/ptrace.c | 5 +
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>>> index cdd5d1d3ae41..3eac38a29863 100644
>>> --- a/arch/powerpc/kernel/ptrace.c
>>> +++ b/arch/powerpc/kernel/ptrace.c
>>> @@ -33,6 +33,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> #include
>>> #include
>>> @@ -298,6 +299,9 @@ int ptrace_get_reg(struct task_struct *task, int regno,
>>> unsigned long *data)
>>> #endif
>>>
>>> if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
>>
>> I would use a variable to store sizeof(struct user_pt_regs) /
>> sizeof(unsigned long).
>
> Right.
>
>>
>>> + regno = array_index_nospec(regno,
>>> + (sizeof(struct user_pt_regs) /
>>> +sizeof(unsigned long)));
>>
>> See the rest of my comments below.
>>
>>> *data = ((unsigned long *)task->thread.regs)[regno];
>>> return 0;
>>> }
>>> @@ -321,6 +325,7 @@ int ptrace_put_reg(struct task_struct *task, int regno,
>>> unsigned long data)
>>> return set_user_dscr(task, data);
>>>
>>> if (regno <= PT_MAX_PUT_REG) {
>>> + regno = array_index_nospec(regno, PT_MAX_PUT_REG);
>>
>> This is wrong. array_index_nospec() will return PT_MAX_PUT_REG - 1 in case
>> regno is equal to
>> PT_MAX_PUT_REG, and this is not what you want.
>
> Right, this is really wrong. It would be correct if the comparison was
> regno < PT_MAX_PUT_REG. Since it is PT_MAX_PUT_REGS is a valid array entry,
> then I need to care about this case also. Doing something as:
>
> regno = array_index_nospec(regno, PT_MAX_PUT_REG + 1);
>
This is the right way.
> Other than that, I think that for the regno = PT_MAX_PUT_REG base,
> array_index_nospec() will redefine regno to 0, not PT_MAX_PUT_REG - 1.
>
>> Similar reasoning applies to the case above.
>
> I understand that the case above does not seem to have the same problem,
> since it does not address over the array as done in the case above. Does it
> make sense?
>
Yeah. I was wrong about that. If regno is out of bounds, array_index_nospec()
will return 0.
> Anyway, this is how I am thinking the v2 of the patch:
>
V2 looks good.
Thanks
--
Gustavo
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index cdd5d1d3ae41..7535f89e08cd 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -274,6 +275,8 @@ static int set_user_trap(struct task_struct *task,
> unsigned long trap)
> */
> int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
> {
> + uns
Re: [PATCH V2] powerpc/ptrace: Mitigate potential Spectre v1
On 1/30/19 6:46 AM, Breno Leitao wrote:
> 'regno' is directly controlled by user space, hence leading to a potential
> exploitation of the Spectre variant 1 vulnerability.
>
> On PTRACE_SETREGS and PTRACE_GETREGS requests, user space passes the
> register number that would be read or written. This register number is
> called 'regno' which is part of the 'addr' syscall parameter.
>
> This 'regno' value is checked against the maximum pt_regs structure size,
> and then used to dereference it, which matches the initial part of a
> Spectre v1 (and Spectre v1.1) attack. The dereferenced value, then,
> is returned to userspace in the GETREGS case.
>
> This patch sanitizes 'regno' before using it to dereference pt_reg.
>
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
>
> [1] https://marc.info/?l=linux-kernel=152449131114778=2
>
> Signed-off-by: Breno Leitao
Acked-by: Gustavo A. R. Silva
> ---
> arch/powerpc/kernel/ptrace.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index cdd5d1d3ae41..7535f89e08cd 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -274,6 +275,8 @@ static int set_user_trap(struct task_struct *task,
> unsigned long trap)
> */
> int ptrace_get_reg(struct task_struct *task, int regno, unsigned long *data)
> {
> + unsigned int regs_max;
> +
> if ((task->thread.regs == NULL) || !data)
> return -EIO;
>
> @@ -297,7 +300,9 @@ int ptrace_get_reg(struct task_struct *task, int regno,
> unsigned long *data)
> }
> #endif
>
> - if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
> + regs_max = sizeof(struct user_pt_regs) / sizeof(unsigned long);
> + if (regno < regs_max) {
> + regno = array_index_nospec(regno, regs_max);
> *data = ((unsigned long *)task->thread.regs)[regno];
> return 0;
> }
> @@ -321,6 +326,7 @@ int ptrace_put_reg(struct task_struct *task, int regno,
> unsigned long data)
> return set_user_dscr(task, data);
>
> if (regno <= PT_MAX_PUT_REG) {
> + regno = array_index_nospec(regno, PT_MAX_PUT_REG + 1);
> ((unsigned long *)task->thread.regs)[regno] = data;
> return 0;
> }
>
Re: [PATCH] powerpc/ptrace: Mitigate potential Spectre v1
On 1/24/19 8:01 AM, Breno Leitao wrote:
> 'regno' is directly controlled by user space, hence leading to a potential
> exploitation of the Spectre variant 1 vulnerability.
>
> On PTRACE_SETREGS and PTRACE_GETREGS requests, user space passes the
> register number that would be read or written. This register number is
> called 'regno' which is part of the 'addr' syscall parameter.
>
> This 'regno' value is checked against the maximum pt_regs structure size,
> and then used to dereference it, which matches the initial part of a
> Spectre v1 (and Spectre v1.1) attack. The dereferenced value, then,
> is returned to userspace in the GETREGS case.
>
Was this reported by any tool?
If so, it might be worth mentioning it.
> This patch sanitizes 'regno' before using it to dereference pt_reg.
>
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
>
> [1] https://marc.info/?l=linux-kernel=152449131114778=2
>
> Signed-off-by: Breno Leitao
> ---
> arch/powerpc/kernel/ptrace.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index cdd5d1d3ae41..3eac38a29863 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -298,6 +299,9 @@ int ptrace_get_reg(struct task_struct *task, int regno,
> unsigned long *data)
> #endif
>
> if (regno < (sizeof(struct user_pt_regs) / sizeof(unsigned long))) {
I would use a variable to store sizeof(struct user_pt_regs) / sizeof(unsigned
long).
> + regno = array_index_nospec(regno,
> + (sizeof(struct user_pt_regs) /
> + sizeof(unsigned long)));
See the rest of my comments below.
> *data = ((unsigned long *)task->thread.regs)[regno];
> return 0;
> }
> @@ -321,6 +325,7 @@ int ptrace_put_reg(struct task_struct *task, int regno,
> unsigned long data)
> return set_user_dscr(task, data);
>
> if (regno <= PT_MAX_PUT_REG) {
> + regno = array_index_nospec(regno, PT_MAX_PUT_REG);
This is wrong. array_index_nospec() will return PT_MAX_PUT_REG - 1 in case
regno is equal to
PT_MAX_PUT_REG, and this is not what you want.
Similar reasoning applies to the case above.
> ((unsigned long *)task->thread.regs)[regno] = data;
> return 0;
> }
>
Thanks
--
Gustavo
Re: powerpc/ps3: Use struct_size() in kzalloc()
On 1/23/19 9:40 PM, Michael Ellerman wrote:
> On Tue, 2019-01-08 at 21:00:10 UTC, "Gustavo A. R. Silva" wrote:
>> One of the more common cases of allocation size calculations is finding the
>> size of a structure that has a zero-sized array at the end, along with memory
>> for some number of elements for that array. For example:
>>
>> struct foo {
>> int stuff;
>> void *entry[];
>> };
>>
>> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
>>
>> Instead of leaving these open-coded and prone to type mistakes, we can now
>> use the new struct_size() helper:
>>
>> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>>
>> This code was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva
>
> Applied to powerpc next, thanks.
>
> https://git.kernel.org/powerpc/c/31367b9a01d6a3f4f77694bd44f547d6
>
Thanks, Michael.
--
Gustavo
Re: [PATCH V4] ASoC: fsl_esai: Fix missing break in switch statement
On 4/10/19 10:24 PM, Gustavo A. R. Silva wrote:
> [+cc lkml]
>
> On 4/10/19 10:05 PM, S.j. Wang wrote:
>> case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
>> independent of each other, so replace fall-through with break.
>>
>> Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
>>
>> Signed-off-by: Shengjiu Wang
>> Acked-by: Nicolin Chen
>> Cc:
>> ---
>> Change in v4
>> - Add Acked-by and cc stable
>> - change the subject
>>
>
> You should preserve the changelog of what has changed in each version, not
> only
> the last changes, so that there is a logical flow and the maintainers do not
> have to dig up previous versions:
>
> Changes in v3:
> - Update subject line.
>
> Changes in v2:
> - Fix "Fixes" tag.
>
>
> Also, for this type of fixes, make sure to always Cc lkml:
> linux-ker...@vger.kernel.org
>
See, these are all the people and lists you should Cc:
$ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback -f
sound/soc/fsl/fsl_esai.c
Timur Tabi (maintainer:FREESCALE SOC SOUND DRIVERS)
Nicolin Chen (maintainer:FREESCALE SOC SOUND DRIVERS)
Xiubo Li (maintainer:FREESCALE SOC SOUND DRIVERS)
Fabio Estevam (reviewer:FREESCALE SOC SOUND DRIVERS)
Liam Girdwood (supporter:SOUND - SOC LAYER / DYNAMIC
AUDIO POWER MANAGEM...)
Mark Brown (supporter:SOUND - SOC LAYER / DYNAMIC AUDIO
POWER MANAGEM...)
Jaroslav Kysela (maintainer:SOUND)
Takashi Iwai (maintainer:SOUND)
alsa-de...@alsa-project.org (moderated list:FREESCALE SOC SOUND DRIVERS)
linuxppc-dev@lists.ozlabs.org (open list:FREESCALE SOC SOUND DRIVERS)
linux-ker...@vger.kernel.org (open list)
Thanks
--
Gustavo
>
>> sound/soc/fsl/fsl_esai.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
>> index 3623aa9a6f2e..15202a637197 100644
>> --- a/sound/soc/fsl/fsl_esai.c
>> +++ b/sound/soc/fsl/fsl_esai.c
>> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
>> *dai, int clk_id,
>> break;
>> case ESAI_HCKT_EXTAL:
>> ecr |= ESAI_ECR_ETI;
>> -/* fall through */
>> +break;
>> case ESAI_HCKR_EXTAL:
>> ecr |= ESAI_ECR_ERI;
>> break;
>>
Re: [PATCH V4] ASoC: fsl_esai: Fix missing break in switch statement
[+cc lkml]
On 4/10/19 10:05 PM, S.j. Wang wrote:
> case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
> independent of each other, so replace fall-through with break.
>
> Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
>
> Signed-off-by: Shengjiu Wang
> Acked-by: Nicolin Chen
> Cc:
> ---
> Change in v4
> - Add Acked-by and cc stable
> - change the subject
>
You should preserve the changelog of what has changed in each version, not only
the last changes, so that there is a logical flow and the maintainers do not
have to dig up previous versions:
Changes in v3:
- Update subject line.
Changes in v2:
- Fix "Fixes" tag.
Also, for this type of fixes, make sure to always Cc lkml:
linux-ker...@vger.kernel.org
Thanks
--
Gustavo
> sound/soc/fsl/fsl_esai.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index 3623aa9a6f2e..15202a637197 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
> *dai, int clk_id,
> break;
> case ESAI_HCKT_EXTAL:
> ecr |= ESAI_ECR_ETI;
> - /* fall through */
> + break;
> case ESAI_HCKR_EXTAL:
> ecr |= ESAI_ECR_ERI;
> break;
>
Re: [EXT] Re: [PATCH V1] ASoC: fsl_esai: replace fall-through with break
Hi Shengjiu,
On 4/8/19 9:54 PM, S.j. Wang wrote:
> Hi Gustavo
>
>>
>>
>> On 4/8/19 4:28 AM, S.j. Wang wrote:
>>> case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
>> independent of
>>> each other, so replace fall-through with break.
>>>
>> If this is correct, then you should use the following "Fixes" tag instead,
>> which is the one that introduced the bug:
>>
>> Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
>>
>>> Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch
>>> fall-through")
>>>
>>
>> because this didn't change any functionality.
>
> Ok, this will be updated.
>
>>
>>> Signed-off-by: Shengjiu Wang
>>> ---
>>> sound/soc/fsl/fsl_esai.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
>>> c7410bbfd2af..bad0dfed6b68 100644
>>> --- a/sound/soc/fsl/fsl_esai.c
>>> +++ b/sound/soc/fsl/fsl_esai.c
>>> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct
>> snd_soc_dai *dai, int clk_id,
>>> break;
>>> case ESAI_HCKT_EXTAL:
>>> ecr |= ESAI_ECR_ETI;
>>
>> Also, you should use a simple assignment operator "=" instead of "|=" in
>> both cases.
>
> The result is same for "=" and "|=", because there is "ecr = 0" in beginning
> of
> This function.
>
Following that same logic, then why not use "+=" instead?
The point is: is "|=" or any other assignment operator other than "=" necessary?
The answer in this case is: no, it is not. So, go for the simple one and avoid
any unnecessary confusion.
Also, there is no need for versioning a patch for it's first revision. If you
receive feedback on a patch and are asked to update it, then you do need to
version the patches that you re-send.
Thanks
--
Gustavo
>>
>>> - /* fall through */
>>> + break;
>>> case ESAI_HCKR_EXTAL:
>>> ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
>>> break;
>>>
>>
>> Thanks
>> --
>> Gustavo
Re: [PATCH V5] ASoC: fsl_esai: Fix missing break in switch statement
Mark,
I wonder if you are going to take this patch.
Thanks
--
Gustavo
On 4/11/19 3:43 AM, S.j. Wang wrote:
> case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
> independent of each other, so replace fall-through with break.
>
> Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> Signed-off-by: Shengjiu Wang
> Acked-by: Nicolin Chen
> Cc:
> ---
> Changes in v5
> - remove new line after Fixes
>
> Changes in v4
> - Add acked-by
>
> Changes in v3
> - Update subject line and cc stable
>
> Changes in v2
> - Fix "Fixes" tag
>
> sound/soc/fsl/fsl_esai.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index 3623aa9a6f2e..15202a637197 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
> *dai, int clk_id,
> break;
> case ESAI_HCKT_EXTAL:
> ecr |= ESAI_ECR_ETI;
> - /* fall through */
> + break;
> case ESAI_HCKR_EXTAL:
> ecr |= ESAI_ECR_ERI;
> break;
>
Re: [PATCH V2] ASoC: fsl_esai: replace fall-through with break
On 4/9/19 9:42 PM, S.j. Wang wrote:
> case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
> each other, so replace fall-through with break.
>
I think you should change the subject line to:
fix missing break in switch statement
...because you are fixing a bug, and it's important to put emphasis on
that in the subject line.
Also, notice that this bug has been out there for more than 5 years now,
so you should also tag this for stable.
Thanks
--
Gustavo
> Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
>
> Signed-off-by: Shengjiu Wang
> ---
> Changes in v2
> - fix the fixes tag.
>
> sound/soc/fsl/fsl_esai.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index c7410bbfd2af..bad0dfed6b68 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
> *dai, int clk_id,
> break;
> case ESAI_HCKT_EXTAL:
> ecr |= ESAI_ECR_ETI;
> - /* fall through */
> + break;
> case ESAI_HCKR_EXTAL:
> ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
> break;
>
Re: [PATCH] powerpc/kvm: Fall through switch case explicitly
On 7/29/19 10:50 PM, Stephen Rothwell wrote: > Hi Gustavo, > > On Mon, 29 Jul 2019 22:30:34 -0500 "Gustavo A. R. Silva" > wrote: >> >> If no one takes it by tomorrow, I'll take it in my tree. > > I am assuming that Michael Ellerman will take it into his fixes tree > and send it to Lunis fairly soon as it actually breaks some powerpc > builds. > Yeah. It seems that now that -Wimplicit-fallthrough has been globally enabled, that's the case for all of these patches. Anyway, I can always take them in my tree if needed. Thanks -- Gustavo
Re: [PATCH] powerpc/kvm: Fall through switch case explicitly
On 7/29/19 3:16 AM, Stephen Rothwell wrote: > Hi Santosh, > > On Mon, 29 Jul 2019 11:25:36 +0530 Santosh Sivaraj wrote: >> >> Implicit fallthrough warning was enabled globally which broke >> the build. Make it explicit with a `fall through` comment. >> >> Signed-off-by: Santosh Sivaraj Reviewed-by: Gustavo A. R. Silva Thanks! -- Gustavo >> --- >> arch/powerpc/kvm/book3s_32_mmu.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/powerpc/kvm/book3s_32_mmu.c >> b/arch/powerpc/kvm/book3s_32_mmu.c >> index 653936177857..18f244aad7aa 100644 >> --- a/arch/powerpc/kvm/book3s_32_mmu.c >> +++ b/arch/powerpc/kvm/book3s_32_mmu.c >> @@ -239,6 +239,7 @@ static int kvmppc_mmu_book3s_32_xlate_pte(struct >> kvm_vcpu *vcpu, gva_t eaddr, >> case 2: >> case 6: >> pte->may_write = true; >> +/* fall through */ >> case 3: >> case 5: >> case 7: >> -- >> 2.20.1 >> > > Thanks > > Reviewed-by: Stephen Rothwell > > This only shows up as a warning in a powerpc allyesconfig build. >
Re: [PATCH] powerpc/kvm: Fall through switch case explicitly
Hi Stephen, On 7/29/19 10:18 PM, Stephen Rothwell wrote: > Hi all, > > On Mon, 29 Jul 2019 18:45:40 -0500 "Gustavo A. R. Silva" > wrote: >> >> On 7/29/19 3:16 AM, Stephen Rothwell wrote: >>> >>> On Mon, 29 Jul 2019 11:25:36 +0530 Santosh Sivaraj >>> wrote: >>>> >>>> Implicit fallthrough warning was enabled globally which broke >>>> the build. Make it explicit with a `fall through` comment. >>>> >>>> Signed-off-by: Santosh Sivaraj >> >> Reviewed-by: Gustavo A. R. Silva >> >> Thanks! >> -- >> Gustavo >> >>>> --- >>>> arch/powerpc/kvm/book3s_32_mmu.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/arch/powerpc/kvm/book3s_32_mmu.c >>>> b/arch/powerpc/kvm/book3s_32_mmu.c >>>> index 653936177857..18f244aad7aa 100644 >>>> --- a/arch/powerpc/kvm/book3s_32_mmu.c >>>> +++ b/arch/powerpc/kvm/book3s_32_mmu.c >>>> @@ -239,6 +239,7 @@ static int kvmppc_mmu_book3s_32_xlate_pte(struct >>>> kvm_vcpu *vcpu, gva_t eaddr, >>>>case 2: >>>>case 6: >>>>pte->may_write = true; >>>> + /* fall through */ >>>>case 3: >>>>case 5: >>>>case 7: >>>> -- >>>> 2.20.1 >>>> >>> >>> Thanks >>> >>> Reviewed-by: Stephen Rothwell >>> >>> This only shows up as a warning in a powerpc allyesconfig build. >>> > > I will apply this to linux-next today (and keep it until it turns up in > some other tree). > If no one takes it by tomorrow, I'll take it in my tree. Thanks! -- Gustavo
[PATCH] dmaengine: fsldma: Mark expected switch fall-through
Mark switch cases where we are expecting to fall through.
Fix the following warning (Building: powerpc-ppa8548_defconfig powerpc):
drivers/dma/fsldma.c: In function ‘fsl_dma_chan_probe’:
drivers/dma/fsldma.c:1165:26: warning: this statement may fall through
[-Wimplicit-fallthrough=]
chan->toggle_ext_pause = fsl_chan_toggle_ext_pause;
~~~^~~
drivers/dma/fsldma.c:1166:2: note: here
case FSL_DMA_IP_83XX:
^~~~
Reported-by: kbuild test robot
Signed-off-by: Gustavo A. R. Silva
---
drivers/dma/fsldma.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 23e0a356f167..ad72b3f42ffa 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1163,6 +1163,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
switch (chan->feature & FSL_DMA_IP_MASK) {
case FSL_DMA_IP_85XX:
chan->toggle_ext_pause = fsl_chan_toggle_ext_pause;
+ /* Fall through */
case FSL_DMA_IP_83XX:
chan->toggle_ext_start = fsl_chan_toggle_ext_start;
chan->set_src_loop_size = fsl_chan_set_src_loop_size;
--
2.22.0
[PATCH] scsi: ibmvfc: Mark expected switch fall-throughs
Mark switch cases where we are expecting to fall through. This patch fixes the following warnings: drivers/scsi/ibmvscsi/ibmvfc.c: In function 'ibmvfc_npiv_login_done': drivers/scsi/ibmvscsi/ibmvfc.c:4022:3: warning: this statement may fall through [-Wimplicit-fallthrough=] ibmvfc_retry_host_init(vhost); ^ drivers/scsi/ibmvscsi/ibmvfc.c:4023:2: note: here case IBMVFC_MAD_DRIVER_FAILED: ^~~~ drivers/scsi/ibmvscsi/ibmvfc.c: In function 'ibmvfc_bsg_request': drivers/scsi/ibmvscsi/ibmvfc.c:1830:11: warning: this statement may fall through [-Wimplicit-fallthrough=] port_id = (bsg_request->rqst_data.h_els.port_id[0] << 16) | ^~~ (bsg_request->rqst_data.h_els.port_id[1] << 8) | bsg_request->rqst_data.h_els.port_id[2]; ~~~ drivers/scsi/ibmvscsi/ibmvfc.c:1833:2: note: here case FC_BSG_RPT_ELS: ^~~~ drivers/scsi/ibmvscsi/ibmvfc.c:1838:11: warning: this statement may fall through [-Wimplicit-fallthrough=] port_id = (bsg_request->rqst_data.h_ct.port_id[0] << 16) | ^~ (bsg_request->rqst_data.h_ct.port_id[1] << 8) | ~~~ bsg_request->rqst_data.h_ct.port_id[2]; ~~ drivers/scsi/ibmvscsi/ibmvfc.c:1841:2: note: here case FC_BSG_RPT_CT: ^~~~ Reported-by: Stephen Rothwell Signed-off-by: Gustavo A. R. Silva --- drivers/scsi/ibmvscsi/ibmvfc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c index 8cdbac076a1b..df897df5cafe 100644 --- a/drivers/scsi/ibmvscsi/ibmvfc.c +++ b/drivers/scsi/ibmvscsi/ibmvfc.c @@ -1830,6 +1830,7 @@ static int ibmvfc_bsg_request(struct bsg_job *job) port_id = (bsg_request->rqst_data.h_els.port_id[0] << 16) | (bsg_request->rqst_data.h_els.port_id[1] << 8) | bsg_request->rqst_data.h_els.port_id[2]; + /* fall through */ case FC_BSG_RPT_ELS: fc_flags = IBMVFC_FC_ELS; break; @@ -1838,6 +1839,7 @@ static int ibmvfc_bsg_request(struct bsg_job *job) port_id = (bsg_request->rqst_data.h_ct.port_id[0] << 16) | (bsg_request->rqst_data.h_ct.port_id[1] << 8) | bsg_request->rqst_data.h_ct.port_id[2]; + /* fall through */ case FC_BSG_RPT_CT: fc_flags = IBMVFC_FC_CT_IU; break; @@ -4020,6 +4022,7 @@ static void ibmvfc_npiv_login_done(struct ibmvfc_event *evt) return; case IBMVFC_MAD_CRQ_ERROR: ibmvfc_retry_host_init(vhost); + /* fall through */ case IBMVFC_MAD_DRIVER_FAILED: ibmvfc_free_event(evt); return; -- 2.22.0
Re: [PATCH] powerpc: sysdev: xive: Fix use true/false for bool type
This is not actually a 'fix'
I suggest you to rephrase the subject in a different way and
remove the word 'fix' from it.
On 10/28/19 22:40, Saurav Girepunje wrote:
> Use true/false for bool return type in xive_spapr_cleanup_queue
> function.
>
How do you find this?
If you used a tool to find this, please mention it.
Thanks
--
Gustavo
> Signed-off-by: Saurav Girepunje
> ---
> arch/powerpc/sysdev/xive/spapr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/sysdev/xive/spapr.c
> b/arch/powerpc/sysdev/xive/spapr.c
> index 33c10749edec..74e3ffae0be6 100644
> --- a/arch/powerpc/sysdev/xive/spapr.c
> +++ b/arch/powerpc/sysdev/xive/spapr.c
> @@ -533,7 +533,7 @@ static void xive_spapr_cleanup_queue(unsigned int cpu,
> struct xive_cpu *xc,
> static bool xive_spapr_match(struct device_node *node)
> {
> /* Ignore cascaded controllers for the moment */
> - return 1;
> + return true;
> }
>
> #ifdef CONFIG_SMP
>
Re: [PATCH] crypto: Replace zero-length array with flexible-array member
On 2/25/20 07:44, Horia Geanta wrote:
> On 2/24/2020 6:18 PM, Gustavo A. R. Silva wrote:
>> The current codebase makes use of the zero-length array language
>> extension to the C90 standard, but the preferred mechanism to declare
>> variable-length types such as these ones is a flexible array member[1][2],
>> introduced in C99:
>>
>> struct foo {
>> int stuff;
>> struct boo array[];
>> };
>>
>> By making use of the mechanism above, we will get a compiler warning
>> in case the flexible array does not occur last in the structure, which
>> will help us prevent some kind of undefined behavior bugs from being
>> inadvertently introduced[3] to the codebase from now on.
>>
>> Also, notice that, dynamic memory allocations won't be affected by
>> this change:
>>
>> "Flexible array members have incomplete type, and so the sizeof operator
>> may not be applied. As a quirk of the original implementation of
>> zero-length arrays, sizeof evaluates to zero."[1]
>>
>> This issue was found with the help of Coccinelle.
>>
>> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
>> [2] https://github.com/KSPP/linux/issues/21
>> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>>
>> Signed-off-by: Gustavo A. R. Silva
> Reviewed-by: Horia Geantă
>
Thank you, Horia.
--
Gustavo
> for caam driver:
>
>> drivers/crypto/caam/caamalg.c | 2 +-
>> drivers/crypto/caam/caamalg_qi.c | 4 ++--
>> drivers/crypto/caam/caamalg_qi2.h | 6 +++---
>> drivers/crypto/caam/caamhash.c | 2 +-
>
> Thanks,
> Horia
>
[PATCH] crypto: Replace zero-length array with flexible-array member
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/crypto/caam/caamalg.c | 2 +-
drivers/crypto/caam/caamalg_qi.c | 4 ++--
drivers/crypto/caam/caamalg_qi2.h | 6 +++---
drivers/crypto/caam/caamhash.c | 2 +-
drivers/crypto/cavium/nitrox/nitrox_main.c | 2 +-
drivers/crypto/chelsio/chcr_core.h | 2 +-
drivers/crypto/mediatek/mtk-sha.c | 2 +-
drivers/crypto/nx/nx.h | 2 +-
drivers/crypto/omap-sham.c | 4 ++--
include/crypto/if_alg.h| 2 +-
10 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 03797f9b1050..b7bb7c30adeb 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -909,7 +909,7 @@ struct skcipher_edesc {
bool bklog;
dma_addr_t sec4_sg_dma;
struct sec4_sg_entry *sec4_sg;
- u32 hw_desc[0];
+ u32 hw_desc[];
};
static void caam_unmap(struct device *dev, struct scatterlist *src,
diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
index 4a29e0ef9d63..27e36bdf6163 100644
--- a/drivers/crypto/caam/caamalg_qi.c
+++ b/drivers/crypto/caam/caamalg_qi.c
@@ -783,7 +783,7 @@ struct aead_edesc {
unsigned int assoclen;
dma_addr_t assoclen_dma;
struct caam_drv_req drv_req;
- struct qm_sg_entry sgt[0];
+ struct qm_sg_entry sgt[];
};
/*
@@ -803,7 +803,7 @@ struct skcipher_edesc {
int qm_sg_bytes;
dma_addr_t qm_sg_dma;
struct caam_drv_req drv_req;
- struct qm_sg_entry sgt[0];
+ struct qm_sg_entry sgt[];
};
static struct caam_drv_ctx *get_drv_ctx(struct caam_ctx *ctx,
diff --git a/drivers/crypto/caam/caamalg_qi2.h
b/drivers/crypto/caam/caamalg_qi2.h
index 706736776b47..f29cb7bd7dd3 100644
--- a/drivers/crypto/caam/caamalg_qi2.h
+++ b/drivers/crypto/caam/caamalg_qi2.h
@@ -114,7 +114,7 @@ struct aead_edesc {
dma_addr_t qm_sg_dma;
unsigned int assoclen;
dma_addr_t assoclen_dma;
- struct dpaa2_sg_entry sgt[0];
+ struct dpaa2_sg_entry sgt[];
};
/*
@@ -132,7 +132,7 @@ struct skcipher_edesc {
dma_addr_t iv_dma;
int qm_sg_bytes;
dma_addr_t qm_sg_dma;
- struct dpaa2_sg_entry sgt[0];
+ struct dpaa2_sg_entry sgt[];
};
/*
@@ -146,7 +146,7 @@ struct ahash_edesc {
dma_addr_t qm_sg_dma;
int src_nents;
int qm_sg_bytes;
- struct dpaa2_sg_entry sgt[0];
+ struct dpaa2_sg_entry sgt[];
};
/**
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 2fe852853d40..943bc0296267 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -536,7 +536,7 @@ struct ahash_edesc {
int sec4_sg_bytes;
bool bklog;
u32 hw_desc[DESC_JOB_IO_LEN_MAX / sizeof(u32)] cacheline_aligned;
- struct sec4_sg_entry sec4_sg[0];
+ struct sec4_sg_entry sec4_sg[];
};
static inline void ahash_unmap(struct device *dev,
diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c
b/drivers/crypto/cavium/nitrox/nitrox_main.c
index c4632d84c9a1..e91be9b8b083 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_main.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
@@ -71,7 +71,7 @@ struct ucode {
char version[VERSION_LEN - 1];
__be32 code_size;
u8 raz[12];
- u64 code[0];
+ u64 code[];
};
/**
diff --git a/drivers/crypto/chelsio/chcr_core.h
b/drivers/crypto/chelsio/chcr_core.h
index b41ef1abfe74..e480096754b5 100644
--- a/drivers/crypto/chelsio/chcr_core.h
+++ b/drivers/crypto/chelsio/chcr_core.h
@@ -68,7 +68,7 @@ struct _key_ctx {
__be32 ctx_hdr;
u8 salt[MAX_SALT];
__be64 iv_to_auth;
- unsigned char key[0];
+ unsigned char key[];
};
#define KEYCTX_TX_WR_IV_S 55
diff --git a/drivers
[PATCH][next] toshiba: Replace zero-length array with flexible-array member
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/net/ethernet/toshiba/ps3_gelic_net.h | 2 +-
drivers/net/ethernet/toshiba/ps3_gelic_wireless.h | 2 +-
drivers/net/ethernet/toshiba/spider_net.h | 2 +-
drivers/net/ethernet/toshiba/tc35815.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
index 805903dbddcc..68f324ed4eaf 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
@@ -308,7 +308,7 @@ struct gelic_port {
struct gelic_card *card;
struct net_device *netdev;
enum gelic_port_type type;
- long priv[0]; /* long for alignment */
+ long priv[]; /* long for alignment */
};
static inline struct gelic_card *port_to_card(struct gelic_port *p)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.h
b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.h
index 4041d946b649..1f203d1ae8db 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.h
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.h
@@ -158,7 +158,7 @@ struct gelic_eurus_scan_info {
__be32 reserved2;
__be32 reserved3;
__be32 reserved4;
- u8 elements[0]; /* ie */
+ u8 elements[]; /* ie */
} __packed;
/* the hypervisor returns bbs up to 16 */
diff --git a/drivers/net/ethernet/toshiba/spider_net.h
b/drivers/net/ethernet/toshiba/spider_net.h
index c0c68cbc898c..05b1a0736835 100644
--- a/drivers/net/ethernet/toshiba/spider_net.h
+++ b/drivers/net/ethernet/toshiba/spider_net.h
@@ -470,7 +470,7 @@ struct spider_net_card {
struct spider_net_extra_stats spider_stats;
/* Must be last item in struct */
- struct spider_net_descr darray[0];
+ struct spider_net_descr darray[];
};
#endif
diff --git a/drivers/net/ethernet/toshiba/tc35815.c
b/drivers/net/ethernet/toshiba/tc35815.c
index 3fd43d30b20d..b50c3ec3495b 100644
--- a/drivers/net/ethernet/toshiba/tc35815.c
+++ b/drivers/net/ethernet/toshiba/tc35815.c
@@ -367,7 +367,7 @@ struct TxFD {
struct RxFD {
struct FDesc fd;
- struct BDesc bd[0]; /* variable length */
+ struct BDesc bd[]; /* variable length */
};
struct FrFD {
--
2.25.0
[PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct qe_firmware.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/soc/fsl/qe/qe.c | 4 ++--
include/soc/fsl/qe/qe.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 447146861c2c1..2df20d6f85fa4 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -448,7 +448,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
unsigned int i;
unsigned int j;
u32 crc;
- size_t calc_size = sizeof(struct qe_firmware);
+ size_t calc_size;
size_t length;
const struct qe_header *hdr;
@@ -480,7 +480,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
}
/* Validate the length and check if there's a CRC */
- calc_size += (firmware->count - 1) * sizeof(struct qe_microcode);
+ calc_size = struct_size(firmware, microcode, firmware->count);
for (i = 0; i < firmware->count; i++)
/*
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index e282ac01ec081..3feddfec9f87d 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -307,7 +307,7 @@ struct qe_firmware {
u8 revision;/* The microcode version revision */
u8 padding; /* Reserved, for alignment */
u8 reserved[4]; /* Reserved, for future expansion */
- } __attribute__ ((packed)) microcode[1];
+ } __packed microcode[];
/* All microcode binaries should be located here */
/* CRC32 should be located here, after the microcode binaries */
} __attribute__ ((packed));
--
2.26.2
[PATCH] powerpc/mm: Replace zero-length array with flexible-array
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
sizeof(flexible-array-member) triggers a warning because flexible array
members have incomplete type[1]. There are some instances of code in
which the sizeof operator is being incorrectly/erroneously applied to
zero-length arrays and the result is zero. Such instances may be hiding
some bugs. So, this work (flexible-array member conversions) will also
help to get completely rid of those sorts of issues.
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/mm/hugetlbpage.c |2 +-
tools/testing/selftests/powerpc/pmu/ebb/trace.h |4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index 33b3461d91e8..d06efb946c7d 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -253,7 +253,7 @@ int __init alloc_bootmem_huge_page(struct hstate *h)
struct hugepd_freelist {
struct rcu_head rcu;
unsigned int index;
- void *ptes[0];
+ void *ptes[];
};
static DEFINE_PER_CPU(struct hugepd_freelist *, hugepd_freelist_cur);
diff --git a/tools/testing/selftests/powerpc/pmu/ebb/trace.h
b/tools/testing/selftests/powerpc/pmu/ebb/trace.h
index 7c0fb5d2bdb1..da2a3be5441f 100644
--- a/tools/testing/selftests/powerpc/pmu/ebb/trace.h
+++ b/tools/testing/selftests/powerpc/pmu/ebb/trace.h
@@ -18,7 +18,7 @@ struct trace_entry
{
u8 type;
u8 length;
- u8 data[0];
+ u8 data[];
};
struct trace_buffer
@@ -26,7 +26,7 @@ struct trace_buffer
u64 size;
bool overflow;
void *tail;
- u8 data[0];
+ u8 data[];
};
struct trace_buffer *trace_buffer_allocate(u64 size);
--
2.26.2
[PATCH] powerpc: Replace zero-length array with flexible-array
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
sizeof(flexible-array-member) triggers a warning because flexible array
members have incomplete type[1]. There are some instances of code in
which the sizeof operator is being incorrectly/erroneously applied to
zero-length arrays and the result is zero. Such instances may be hiding
some bugs. So, this work (flexible-array member conversions) will also
help to get completely rid of those sorts of issues.
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/platforms/powermac/nvram.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powermac/nvram.c
b/arch/powerpc/platforms/powermac/nvram.c
index dc7a5bae8f1c..853ccc4480e2 100644
--- a/arch/powerpc/platforms/powermac/nvram.c
+++ b/arch/powerpc/platforms/powermac/nvram.c
@@ -55,7 +55,7 @@ struct chrp_header {
u8 cksum;
u16 len;
char name[12];
- u8 data[0];
+ u8 data[];
};
struct core99_header {
[PATCH] treewide: Replace zero-length array with flexible-array
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
sizeof(flexible-array-member) triggers a warning because flexible array
members have incomplete type[1]. There are some instances of code in
which the sizeof operator is being incorrectly/erroneously applied to
zero-length arrays and the result is zero. Such instances may be hiding
some bugs. So, this work (flexible-array member conversions) will also
help to get completely rid of those sorts of issues.
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
include/linux/fsl/bestcomm/bestcomm.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/fsl/bestcomm/bestcomm.h
b/include/linux/fsl/bestcomm/bestcomm.h
index a0e2e6b19b57..154e541ce57e 100644
--- a/include/linux/fsl/bestcomm/bestcomm.h
+++ b/include/linux/fsl/bestcomm/bestcomm.h
@@ -27,7 +27,7 @@
*/
struct bcom_bd {
u32 status;
- u32 data[0];/* variable payload size */
+ u32 data[]; /* variable payload size */
};
/* */
Re: [PATCH] soc: fsl: qe: Replace one-element array and use struct_size() helper
On Wed, May 20, 2020 at 06:52:21PM -0500, Li Yang wrote:
> On Mon, May 18, 2020 at 5:57 PM Kees Cook wrote:
> >
> > On Mon, May 18, 2020 at 05:19:04PM -0500, Gustavo A. R. Silva wrote:
> > > The current codebase makes use of one-element arrays in the following
> > > form:
> > >
> > > struct something {
> > > int length;
> > > u8 data[1];
> > > };
> > >
> > > struct something *instance;
> > >
> > > instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> > > instance->length = size;
> > > memcpy(instance->data, source, size);
> > >
> > > but the preferred mechanism to declare variable-length types such as
> > > these ones is a flexible array member[1][2], introduced in C99:
> > >
> > > struct foo {
> > > int stuff;
> > > struct boo array[];
> > > };
> > >
> > > By making use of the mechanism above, we will get a compiler warning
> > > in case the flexible array does not occur last in the structure, which
> > > will help us prevent some kind of undefined behavior bugs from being
> > > inadvertently introduced[3] to the codebase from now on. So, replace
> > > the one-element array with a flexible-array member.
> > >
> > > Also, make use of the new struct_size() helper to properly calculate the
> > > size of struct qe_firmware.
> > >
> > > This issue was found with the help of Coccinelle and, audited and fixed
> > > _manually_.
> > >
> > > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> > > [2] https://github.com/KSPP/linux/issues/21
> > > [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> > >
> > > Signed-off-by: Gustavo A. R. Silva
> > > ---
> > > drivers/soc/fsl/qe/qe.c | 4 ++--
> > > include/soc/fsl/qe/qe.h | 2 +-
> > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
> > > index 447146861c2c1..2df20d6f85fa4 100644
> > > --- a/drivers/soc/fsl/qe/qe.c
> > > +++ b/drivers/soc/fsl/qe/qe.c
> > > @@ -448,7 +448,7 @@ int qe_upload_firmware(const struct qe_firmware
> > > *firmware)
> > > unsigned int i;
> > > unsigned int j;
> > > u32 crc;
> > > - size_t calc_size = sizeof(struct qe_firmware);
> > > + size_t calc_size;
> > > size_t length;
> > > const struct qe_header *hdr;
> > >
> > > @@ -480,7 +480,7 @@ int qe_upload_firmware(const struct qe_firmware
> > > *firmware)
> > > }
> > >
> > > /* Validate the length and check if there's a CRC */
> > > - calc_size += (firmware->count - 1) * sizeof(struct qe_microcode);
> > > + calc_size = struct_size(firmware, microcode, firmware->count);
> > >
> > > for (i = 0; i < firmware->count; i++)
> > > /*
> > > diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
> > > index e282ac01ec081..3feddfec9f87d 100644
> > > --- a/include/soc/fsl/qe/qe.h
> > > +++ b/include/soc/fsl/qe/qe.h
> > > @@ -307,7 +307,7 @@ struct qe_firmware {
> > > u8 revision;/* The microcode version revision */
> > > u8 padding; /* Reserved, for alignment */
> > > u8 reserved[4]; /* Reserved, for future expansion */
> > > - } __attribute__ ((packed)) microcode[1];
> > > + } __packed microcode[];
> > > /* All microcode binaries should be located here */
> > > /* CRC32 should be located here, after the microcode binaries */
> > > } __attribute__ ((packed));
> > > --
> > > 2.26.2
> > >
> >
> > Hm, looking at this code, I see a few other things that need to be
> > fixed:
> >
> > 1) drivers/tty/serial/ucc_uart.c does not do a be32_to_cpu() conversion
> >on the length test (understandably, a little-endian system has never run
> >this code since it's ppc specific), but it's still wrong:
> >
> > if (firmware->header.length != fw->size) {
> >
> >compare to the firmware loader:
> >
> > length = be32_to_cpu(hdr->length);
> >
> > 2) drivers/soc/fsl/qe/qe.c does not perform bounds checking on the
> >per-microcode offsets, so the uploader might send data outside the
> >firmware
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On 9/9/20 15:06, Joe Perches wrote:
> fallthrough to a separate case/default label break; isn't very readable.
>
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
>
> Found using:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
>
> Miscellanea:
>
> o Move or coalesce a couple label blocks above a default: block.
>
> Signed-off-by: Joe Perches
Acked-by: Gustavo A. R. Silva
Thanks
--
Gustavo
> ---
>
> Compiled allyesconfig x86-64 only.
> A few files for other arches were not compiled.
>
> arch/arm/mach-mmp/pm-pxa910.c | 2 +-
> arch/arm64/kvm/handle_exit.c | 2 +-
> arch/mips/kernel/cpu-probe.c | 2 +-
> arch/mips/math-emu/cp1emu.c | 2 +-
> arch/s390/pci/pci.c | 2 +-
> crypto/tcrypt.c | 4 ++--
> drivers/ata/sata_mv.c | 2 +-
> drivers/atm/lanai.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
> drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c | 2 +-
> drivers/hid/wacom_wac.c | 2 +-
> drivers/i2c/busses/i2c-i801.c | 2 +-
> drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
> drivers/infiniband/ulp/rtrs/rtrs-srv.c| 6 +++---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
> drivers/irqchip/irq-vic.c | 4 ++--
> drivers/md/dm.c | 2 +-
> drivers/media/dvb-frontends/drxd_hard.c | 2 +-
> drivers/media/i2c/ov5640.c| 2 +-
> drivers/media/i2c/ov6650.c| 5 ++---
> drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
> drivers/media/i2c/tvp5150.c | 2 +-
> drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
> drivers/media/usb/cpia2/cpia2_core.c | 2 +-
> drivers/mfd/iqs62x.c | 3 +--
> drivers/mmc/host/atmel-mci.c | 2 +-
> drivers/mtd/nand/raw/nandsim.c| 2 +-
> drivers/net/ethernet/intel/e1000e/phy.c | 2 +-
> drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_adminq.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
> drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
> drivers/net/ethernet/intel/igb/e1000_phy.c| 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c| 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 2 +-
> drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 2 +-
> drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
> drivers/net/ethernet/sfc/falcon/farch.c | 2 +-
> drivers/net/ethernet/sfc/farch.c | 2 +-
> drivers/net/phy/adin.c| 3 +--
> drivers/net/usb/pegasus.c | 4 ++--
> drivers/net/usb/usbnet.c | 2 +-
> drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
> drivers/net/wireless/mediatek/mt7601u/dma.c | 8
> drivers/nvme/host/core.c | 12 ++--
> drivers/pcmcia/db1xxx_ss.c| 4 ++--
> drivers/power/supply/abx500_chargalg.c| 2 +-
> drivers/power/supply/charger-manager.c| 2 +-
> drivers/rtc/rtc-pcf85063.c| 2 +-
> drivers/s390/scsi/zfcp_fsf.c | 2 +-
> drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++--
> drivers/scsi/aic94xx/aic94xx_tmf.c| 2 +-
> drivers/scsi/lpfc/lpfc_sli.c | 2 +-
> drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
> drivers/scsi/sr.c | 2 +-
> drivers/tty/serial/sunsu.c| 2
Re: [GIT PULL] fallthrough pseudo-keyword macro conversions for 5.9-rc3
Hi Nathan, On 8/24/20 14:43, Nathan Chancellor wrote: >> Gustavo A. R. Silva (1): >> treewide: Use fallthrough pseudo-keyword > > $ scripts/config --file arch/powerpc/configs/powernv_defconfig -e KERNEL_XZ > > $ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc64le-linux- distclean > powernv_defconfig zImage > ... > In file included from arch/powerpc/boot/../../../lib/decompress_unxz.c:234, > from arch/powerpc/boot/decompress.c:38: > arch/powerpc/boot/../../../lib/xz/xz_dec_stream.c: In function 'dec_main': > arch/powerpc/boot/../../../lib/xz/xz_dec_stream.c:586:4: error: 'fallthrough' > undeclared (first use in this function) > 586 |fallthrough; > |^~~ > arch/powerpc/boot/../../../lib/xz/xz_dec_stream.c:586:4: note: each > undeclared identifier is reported only once for each function it appears in > In file included from arch/powerpc/boot/../../../lib/decompress_unxz.c:235, > from arch/powerpc/boot/decompress.c:38: > arch/powerpc/boot/../../../lib/xz/xz_dec_lzma2.c: In function > 'xz_dec_lzma2_run': > arch/powerpc/boot/../../../lib/xz/xz_dec_lzma2.c:1046:4: error: 'fallthrough' > undeclared (first use in this function) > 1046 |fallthrough; > |^~~ > make[2]: *** [arch/powerpc/boot/Makefile:215: arch/powerpc/boot/decompress.o] > Error 1 > make[2]: Target 'arch/powerpc/boot/zImage' not remade because of errors. > make[1]: *** [arch/powerpc/Makefile:295: zImage] Error 2 > make: *** [Makefile:335: __build_one_by_one] Error 2 > make: Target 'distclean' not remade because of errors. > make: Target 'powernv_defconfig' not remade because of errors. > make: Target 'zImage' not remade because of errors. > > This will end up affecting distribution configurations such as Debian > and OpenSUSE according to my testing. I am not sure what the solution > is, the PowerPC wrapper does not set -D__KERNEL__ so I am not sure that > compiler_attributes.h can be safely included. Adding Michael and > linuxppc-dev to CC. > Thanks for the report. I think, for now, the best solution is to use /* fall through */ comments instead of the pseudo-keyword in lib/ I'll send a fix for that right away. Thanks -- Gustavo
[PATCH][next] powerpc: Use fallthrough pseudo-keyword
Replace the existing /* fall through */ comments and its variants with
the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
fall-through markings when it is the case.
[1]
https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/kernel/align.c | 8
arch/powerpc/platforms/powermac/feature.c | 2 +-
arch/powerpc/platforms/powernv/opal-async.c | 2 +-
arch/powerpc/platforms/pseries/hvcserver.c | 2 +-
arch/powerpc/xmon/xmon.c| 2 +-
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 1f1ce8b86d5b..c7797eb958c7 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -178,11 +178,11 @@ static int emulate_spe(struct pt_regs *regs, unsigned int
reg,
ret |= __get_user_inatomic(temp.v[1], p++);
ret |= __get_user_inatomic(temp.v[2], p++);
ret |= __get_user_inatomic(temp.v[3], p++);
- /* fall through */
+ fallthrough;
case 4:
ret |= __get_user_inatomic(temp.v[4], p++);
ret |= __get_user_inatomic(temp.v[5], p++);
- /* fall through */
+ fallthrough;
case 2:
ret |= __get_user_inatomic(temp.v[6], p++);
ret |= __get_user_inatomic(temp.v[7], p++);
@@ -263,11 +263,11 @@ static int emulate_spe(struct pt_regs *regs, unsigned int
reg,
ret |= __put_user_inatomic(data.v[1], p++);
ret |= __put_user_inatomic(data.v[2], p++);
ret |= __put_user_inatomic(data.v[3], p++);
- /* fall through */
+ fallthrough;
case 4:
ret |= __put_user_inatomic(data.v[4], p++);
ret |= __put_user_inatomic(data.v[5], p++);
- /* fall through */
+ fallthrough;
case 2:
ret |= __put_user_inatomic(data.v[6], p++);
ret |= __put_user_inatomic(data.v[7], p++);
diff --git a/arch/powerpc/platforms/powermac/feature.c
b/arch/powerpc/platforms/powermac/feature.c
index 181caa3f6717..5c77b9a24c0e 100644
--- a/arch/powerpc/platforms/powermac/feature.c
+++ b/arch/powerpc/platforms/powermac/feature.c
@@ -1465,7 +1465,7 @@ static long g5_i2s_enable(struct device_node *node, long
param, long value)
case 2:
if (macio->type == macio_shasta)
break;
- /* fall through */
+ fallthrough;
default:
return -ENODEV;
}
diff --git a/arch/powerpc/platforms/powernv/opal-async.c
b/arch/powerpc/platforms/powernv/opal-async.c
index 1656e8965d6b..c094fdf5825c 100644
--- a/arch/powerpc/platforms/powernv/opal-async.c
+++ b/arch/powerpc/platforms/powernv/opal-async.c
@@ -104,7 +104,7 @@ static int __opal_async_release_token(int token)
*/
case ASYNC_TOKEN_DISPATCHED:
opal_async_tokens[token].state = ASYNC_TOKEN_ABANDONED;
- /* Fall through */
+ fallthrough;
default:
rc = 1;
}
diff --git a/arch/powerpc/platforms/pseries/hvcserver.c
b/arch/powerpc/platforms/pseries/hvcserver.c
index 267139b13530..96e18d3b2fcf 100644
--- a/arch/powerpc/platforms/pseries/hvcserver.c
+++ b/arch/powerpc/platforms/pseries/hvcserver.c
@@ -45,7 +45,7 @@ static int hvcs_convert(long to_convert)
case H_LONG_BUSY_ORDER_10_SEC:
case H_LONG_BUSY_ORDER_100_SEC:
return -EBUSY;
- case H_FUNCTION: /* fall through */
+ case H_FUNCTION:
default:
return -EPERM;
}
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 8fb1f857c11c..ed1a9f43709d 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -4278,7 +4278,7 @@ static int do_spu_cmd(void)
subcmd = inchar();
if (isxdigit(subcmd) || subcmd == '\n')
termch = subcmd;
- /* fall through */
+ fallthrough;
case 'f':
scanhex();
if (num >= XMON_NUM_SPUS || !spu_info[num].spu) {
--
2.27.0
[PATCH][next] dmaengine: Use fallthrough pseudo-keyword
Replace the existing /* fall through */ comments and its variants with
the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
fall-through markings when it is the case.
[1]
https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
Signed-off-by: Gustavo A. R. Silva
---
drivers/dma/amba-pl08x.c| 10 +-
drivers/dma/fsldma.c| 2 +-
drivers/dma/imx-dma.c | 2 +-
drivers/dma/iop-adma.h | 12 ++--
drivers/dma/nbpfaxi.c | 2 +-
drivers/dma/pl330.c | 10 +++---
drivers/dma/sh/shdma-base.c | 2 +-
7 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 9adc7a2fa3d3..a24882ba3764 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1767,7 +1767,7 @@ static u32 pl08x_memcpy_cctl(struct pl08x_driver_data
*pl08x)
default:
dev_err(>adev->dev,
"illegal burst size for memcpy, set to 1\n");
- /* Fall through */
+ fallthrough;
case PL08X_BURST_SZ_1:
cctl |= PL080_BSIZE_1 << PL080_CONTROL_SB_SIZE_SHIFT |
PL080_BSIZE_1 << PL080_CONTROL_DB_SIZE_SHIFT;
@@ -1806,7 +1806,7 @@ static u32 pl08x_memcpy_cctl(struct pl08x_driver_data
*pl08x)
default:
dev_err(>adev->dev,
"illegal bus width for memcpy, set to 8 bits\n");
- /* Fall through */
+ fallthrough;
case PL08X_BUS_WIDTH_8_BITS:
cctl |= PL080_WIDTH_8BIT << PL080_CONTROL_SWIDTH_SHIFT |
PL080_WIDTH_8BIT << PL080_CONTROL_DWIDTH_SHIFT;
@@ -1850,7 +1850,7 @@ static u32 pl08x_ftdmac020_memcpy_cctl(struct
pl08x_driver_data *pl08x)
default:
dev_err(>adev->dev,
"illegal bus width for memcpy, set to 8 bits\n");
- /* Fall through */
+ fallthrough;
case PL08X_BUS_WIDTH_8_BITS:
cctl |= PL080_WIDTH_8BIT << FTDMAC020_LLI_SRC_WIDTH_SHIFT |
PL080_WIDTH_8BIT << FTDMAC020_LLI_DST_WIDTH_SHIFT;
@@ -2612,7 +2612,7 @@ static int pl08x_of_probe(struct amba_device *adev,
switch (val) {
default:
dev_err(>dev, "illegal burst size for memcpy, set to
1\n");
- /* Fall through */
+ fallthrough;
case 1:
pd->memcpy_burst_size = PL08X_BURST_SZ_1;
break;
@@ -2647,7 +2647,7 @@ static int pl08x_of_probe(struct amba_device *adev,
switch (val) {
default:
dev_err(>dev, "illegal bus width for memcpy, set to 8
bits\n");
- /* Fall through */
+ fallthrough;
case 8:
pd->memcpy_bus_width = PL08X_BUS_WIDTH_8_BITS;
break;
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index ad72b3f42ffa..e342cf52d296 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1163,7 +1163,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
switch (chan->feature & FSL_DMA_IP_MASK) {
case FSL_DMA_IP_85XX:
chan->toggle_ext_pause = fsl_chan_toggle_ext_pause;
- /* Fall through */
+ fallthrough;
case FSL_DMA_IP_83XX:
chan->toggle_ext_start = fsl_chan_toggle_ext_start;
chan->set_src_loop_size = fsl_chan_set_src_loop_size;
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index 5c0fb3134825..88717506c1f6 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -556,7 +556,7 @@ static int imxdma_xfer_desc(struct imxdma_desc *d)
* We fall-through here intentionally, since a 2D transfer is
* similar to MEMCPY just adding the 2D slot configuration.
*/
- /* Fall through */
+ fallthrough;
case IMXDMA_DESC_MEMCPY:
imx_dmav1_writel(imxdma, d->src, DMA_SAR(imxdmac->channel));
imx_dmav1_writel(imxdma, d->dest, DMA_DAR(imxdmac->channel));
diff --git a/drivers/dma/iop-adma.h b/drivers/dma/iop-adma.h
index c499c9578f00..d44eabb6f5eb 100644
--- a/drivers/dma/iop-adma.h
+++ b/drivers/dma/iop-adma.h
@@ -496,7 +496,7 @@ iop3xx_desc_init_xor(struct iop3xx_desc_aau *hw_desc, int
src_cnt,
}
hw_desc->src_edc[AAU_EDCR2_IDX].e_desc_ctrl = edcr;
src_cnt = 24;
- /* fall through */
+ fallthrough;
case 17 ... 24:
if (!u_desc_ctrl.field.blk_ctrl) {
hw_desc->src_edc[AAU_EDCR2_IDX].e_desc_ctrl = 0;
@@ -510,7 +510,7 @@ iop3xx_desc_init_xor(struct iop3
Re: [PATCH 2/3] Revert "lib: Revert use of fallthrough pseudo-keyword in lib/"
On Sun, Nov 15, 2020 at 08:35:31PM -0800, Nick Desaulniers wrote:
> This reverts commit 6a9dc5fd6170 ("lib: Revert use of fallthrough
> pseudo-keyword in lib/")
>
> Now that we can build arch/powerpc/boot/ free of -Wimplicit-fallthrough,
> re-enable these fixes for lib/.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/236
> Signed-off-by: Nick Desaulniers
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
> ---
> lib/asn1_decoder.c | 4 ++--
> lib/assoc_array.c | 2 +-
> lib/bootconfig.c| 4 ++--
> lib/cmdline.c | 10 +-
> lib/dim/net_dim.c | 2 +-
> lib/dim/rdma_dim.c | 4 ++--
> lib/glob.c | 2 +-
> lib/siphash.c | 36 ++--
> lib/ts_fsm.c| 2 +-
> lib/vsprintf.c | 14 +++---
> lib/xz/xz_dec_lzma2.c | 4 ++--
> lib/xz/xz_dec_stream.c | 16
> lib/zstd/bitstream.h| 10 +-
> lib/zstd/compress.c | 2 +-
> lib/zstd/decompress.c | 12 ++--
> lib/zstd/huf_compress.c | 4 ++--
> 16 files changed, 64 insertions(+), 64 deletions(-)
>
> diff --git a/lib/asn1_decoder.c b/lib/asn1_decoder.c
> index 58f72b25f8e9..13da529e2e72 100644
> --- a/lib/asn1_decoder.c
> +++ b/lib/asn1_decoder.c
> @@ -381,7 +381,7 @@ int asn1_ber_decoder(const struct asn1_decoder *decoder,
> case ASN1_OP_END_SET_ACT:
> if (unlikely(!(flags & FLAG_MATCHED)))
> goto tag_mismatch;
> - /* fall through */
> + fallthrough;
>
> case ASN1_OP_END_SEQ:
> case ASN1_OP_END_SET_OF:
> @@ -448,7 +448,7 @@ int asn1_ber_decoder(const struct asn1_decoder *decoder,
> pc += asn1_op_lengths[op];
> goto next_op;
> }
> - /* fall through */
> + fallthrough;
>
> case ASN1_OP_ACT:
> ret = actions[machine[pc + 1]](context, hdr, tag, data + tdp,
> len);
> diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> index 6f4bcf524554..04c98799c3ba 100644
> --- a/lib/assoc_array.c
> +++ b/lib/assoc_array.c
> @@ -1113,7 +1113,7 @@ struct assoc_array_edit *assoc_array_delete(struct
> assoc_array *array,
> index_key))
> goto found_leaf;
> }
> - /* fall through */
> + fallthrough;
> case assoc_array_walk_tree_empty:
> case assoc_array_walk_found_wrong_shortcut:
> default:
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index 649ed44f199c..9f8c70a98fcf 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -827,7 +827,7 @@ int __init xbc_init(char *buf, const char **emsg, int
> *epos)
> q - 2);
> break;
> }
> - /* fall through */
> + fallthrough;
> case '=':
> ret = xbc_parse_kv(, q, c);
> break;
> @@ -836,7 +836,7 @@ int __init xbc_init(char *buf, const char **emsg, int
> *epos)
> break;
> case '#':
> q = skip_comment(q);
> - /* fall through */
> + fallthrough;
> case ';':
> case '\n':
> ret = xbc_parse_key(, q);
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index 9e186234edc0..46f2cb4ce6d1 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -144,23 +144,23 @@ unsigned long long memparse(const char *ptr, char
> **retptr)
> case 'E':
> case 'e':
> ret <<= 10;
> - /* fall through */
> + fallthrough;
> case 'P':
> case 'p':
> ret <<= 10;
> - /* fall through */
> + fallthrough;
> case 'T':
> case 't':
> ret <<= 10;
> - /* fall through */
> + fallthrough;
> case 'G':
> case 'g':
> ret <<= 10;
> - /* fall through */
> + fallthrough;
> case 'M':
> case 'm':
> ret <<= 10;
> - /* fall through */
> + fallthrough;
> case 'K':
> case 'k':
> ret <<= 10;
> diff --git a/lib/dim/net_dim.c b/lib/dim/net_dim.c
> index a4db51c21266..06811d866775 100644
> --- a/lib/dim/net_dim.c
> +++ b/lib/dim/net_dim.c
> @@ -233,7 +233,7 @@ void
Re: [PATCH 1/3] powerpc: boot: include compiler_attributes.h
On Sun, Nov 15, 2020 at 08:35:30PM -0800, Nick Desaulniers wrote: > The kernel uses `-include` to include include/linux/compiler_types.h > into all translation units (see scripts/Makefile.lib), which #includes > compiler_attributes.h. > > arch/powerpc/boot/ uses different compiler flags from the rest of the > kernel. As such, it doesn't contain the definitions from these headers, > and redefines a few that it needs. > > For the purpose of enabling -Wimplicit-fallthrough for ppc, include > compiler_types.h via `-include`. > > Link: https://github.com/ClangBuiltLinux/linux/issues/236 > Signed-off-by: Nick Desaulniers Acked-by: Gustavo A. R. Silva Thanks, Nick. -- Gustavo > --- > We could just `#include "include/linux/compiler_types.h"` in the few .c > sources used from lib/ (there are proper header guards in > compiler_types.h). > > It was also noted in 6a9dc5fd6170 that we could -D__KERNEL__ and > -include compiler_types.h like the main kernel does, though testing that > produces a whole sea of warnings to cleanup. This approach is minimally > invasive. > > arch/powerpc/boot/Makefile | 1 + > arch/powerpc/boot/decompress.c | 1 - > 2 files changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile > index f8ce6d2dde7b..1659963a8f1d 100644 > --- a/arch/powerpc/boot/Makefile > +++ b/arch/powerpc/boot/Makefile > @@ -31,6 +31,7 @@ endif > BOOTCFLAGS:= -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \ >-fno-strict-aliasing -O2 -msoft-float -mno-altivec -mno-vsx \ >-pipe -fomit-frame-pointer -fno-builtin -fPIC -nostdinc \ > + -include $(srctree)/include/linux/compiler_attributes.h \ >$(LINUXINCLUDE) > > ifdef CONFIG_PPC64_BOOT_WRAPPER > diff --git a/arch/powerpc/boot/decompress.c b/arch/powerpc/boot/decompress.c > index 8bf39ef7d2df..6098b879ac97 100644 > --- a/arch/powerpc/boot/decompress.c > +++ b/arch/powerpc/boot/decompress.c > @@ -21,7 +21,6 @@ > > #define STATIC static > #define INIT > -#define __always_inline inline > > /* > * The build process will copy the required zlib source files and headers > -- > 2.29.2.299.gdc1121823c-goog >
Re: [PATCH 3/3] powerpc: fix -Wimplicit-fallthrough
On Sun, Nov 15, 2020 at 08:35:32PM -0800, Nick Desaulniers wrote: > The "fallthrough" pseudo-keyword was added as a portable way to denote > intentional fallthrough. Clang will still warn on cases where there is a > fallthrough to an immediate break. Add explicit breaks for those cases. > > Link: https://github.com/ClangBuiltLinux/linux/issues/236 > Signed-off-by: Nick Desaulniers Reviewed-by: Gustavo A. R. Silva Thanks -- Gustavo > --- > arch/powerpc/kernel/prom_init.c | 1 + > arch/powerpc/kernel/uprobes.c | 1 + > arch/powerpc/perf/imc-pmu.c | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c > index 38ae5933d917..e9d4eb6144e1 100644 > --- a/arch/powerpc/kernel/prom_init.c > +++ b/arch/powerpc/kernel/prom_init.c > @@ -355,6 +355,7 @@ static int __init prom_strtobool(const char *s, bool *res) > default: > break; > } > + break; > default: > break; > } > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > index d200e7df7167..e8a63713e655 100644 > --- a/arch/powerpc/kernel/uprobes.c > +++ b/arch/powerpc/kernel/uprobes.c > @@ -141,6 +141,7 @@ int arch_uprobe_exception_notify(struct notifier_block > *self, > case DIE_SSTEP: > if (uprobe_post_sstep_notifier(regs)) > return NOTIFY_STOP; > + break; > default: > break; > } > diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c > index 7b25548ec42b..e106909ff9c3 100644 > --- a/arch/powerpc/perf/imc-pmu.c > +++ b/arch/powerpc/perf/imc-pmu.c > @@ -1500,6 +1500,7 @@ static int update_pmu_ops(struct imc_pmu *pmu) > pmu->pmu.stop = trace_imc_event_stop; > pmu->pmu.read = trace_imc_event_read; > pmu->attr_groups[IMC_FORMAT_ATTR] = _imc_format_group; > + break; > default: > break; > } > -- > 2.29.2.299.gdc1121823c-goog >
Re: [PATCH v2 0/3] PPC: Fix -Wimplicit-fallthrough for clang
Nick,
On Tue, Nov 17, 2020 at 04:07:48PM -0800, Nick Desaulniers wrote:
> While cleaning up the last few -Wimplicit-fallthrough warnings in tree
> for Clang, I noticed
> commit 6a9dc5fd6170d ("lib: Revert use of fallthrough pseudo-keyword in lib/")
> which seemed to undo a bunch of fixes in lib/ due to breakage in
> arch/powerpc/boot/ not including compiler_types.h. We don't need
> compiler_types.h for the definition of `fallthrough`, simply
> compiler_attributes.h. Include that, revert the revert to lib/, and fix
> the last remaining cases I observed for powernv_defconfig.
I've added the series to my -next tree, together with Miguel's
suggestions.
Thanks for the Acks and comments, Michael.
--
Gustavo
Re: [PATCH 2/3] Revert "lib: Revert use of fallthrough pseudo-keyword in lib/"
On Tue, Nov 17, 2020 at 11:10:26AM -0800, Nick Desaulniers wrote:
> On Mon, Nov 16, 2020 at 7:02 PM Nathan Chancellor
> wrote:
> >
> > On Sun, Nov 15, 2020 at 08:35:31PM -0800, Nick Desaulniers wrote:
> > > This reverts commit 6a9dc5fd6170 ("lib: Revert use of fallthrough
> > > pseudo-keyword in lib/")
>
> Gustavo, whose tree did 6a9dc5fd6170 and df561f6688fe go up to
Mine.
> mainline in? I'm not sure whether you or MPE (ppc) or someone else
> should pick it this series?
I'm happy to take this series in my tree. I'm planing to send a
pull-request for -rc5 with more related changes. So, I can include
this in the same PR.
In the meantime I'll add this to my testing tree, so it can be
build-tested by the 0-day folks. :)
Thanks
--
Gustavo
>
> > >
> > > Now that we can build arch/powerpc/boot/ free of -Wimplicit-fallthrough,
> > > re-enable these fixes for lib/.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/236
> > > Signed-off-by: Nick Desaulniers
> >
> > Reviewed-by: Nathan Chancellor
> > Tested-by: Nathan Chancellor
> >
> > > ---
> > > lib/asn1_decoder.c | 4 ++--
> > > lib/assoc_array.c | 2 +-
> > > lib/bootconfig.c| 4 ++--
> > > lib/cmdline.c | 10 +-
> > > lib/dim/net_dim.c | 2 +-
> > > lib/dim/rdma_dim.c | 4 ++--
> > > lib/glob.c | 2 +-
> > > lib/siphash.c | 36 ++--
> > > lib/ts_fsm.c| 2 +-
> > > lib/vsprintf.c | 14 +++---
> > > lib/xz/xz_dec_lzma2.c | 4 ++--
> > > lib/xz/xz_dec_stream.c | 16
> > > lib/zstd/bitstream.h| 10 +-
> > > lib/zstd/compress.c | 2 +-
> > > lib/zstd/decompress.c | 12 ++--
> > > lib/zstd/huf_compress.c | 4 ++--
> > > 16 files changed, 64 insertions(+), 64 deletions(-)
> > >
> > > diff --git a/lib/asn1_decoder.c b/lib/asn1_decoder.c
> > > index 58f72b25f8e9..13da529e2e72 100644
> > > --- a/lib/asn1_decoder.c
> > > +++ b/lib/asn1_decoder.c
> > > @@ -381,7 +381,7 @@ int asn1_ber_decoder(const struct asn1_decoder
> > > *decoder,
> > > case ASN1_OP_END_SET_ACT:
> > > if (unlikely(!(flags & FLAG_MATCHED)))
> > > goto tag_mismatch;
> > > - /* fall through */
> > > + fallthrough;
> > >
> > > case ASN1_OP_END_SEQ:
> > > case ASN1_OP_END_SET_OF:
> > > @@ -448,7 +448,7 @@ int asn1_ber_decoder(const struct asn1_decoder
> > > *decoder,
> > > pc += asn1_op_lengths[op];
> > > goto next_op;
> > > }
> > > - /* fall through */
> > > + fallthrough;
> > >
> > > case ASN1_OP_ACT:
> > > ret = actions[machine[pc + 1]](context, hdr, tag, data +
> > > tdp, len);
> > > diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> > > index 6f4bcf524554..04c98799c3ba 100644
> > > --- a/lib/assoc_array.c
> > > +++ b/lib/assoc_array.c
> > > @@ -1113,7 +1113,7 @@ struct assoc_array_edit *assoc_array_delete(struct
> > > assoc_array *array,
> > > index_key))
> > > goto found_leaf;
> > > }
> > > - /* fall through */
> > > + fallthrough;
> > > case assoc_array_walk_tree_empty:
> > > case assoc_array_walk_found_wrong_shortcut:
> > > default:
> > > diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> > > index 649ed44f199c..9f8c70a98fcf 100644
> > > --- a/lib/bootconfig.c
> > > +++ b/lib/bootconfig.c
> > > @@ -827,7 +827,7 @@ int __init xbc_init(char *buf, const char **emsg, int
> > > *epos)
> > > q - 2);
> > > break;
> > > }
> > > - /* fall through */
> > > + fallthrough;
> > > case '=':
> > > ret = xbc_parse_kv(, q, c);
> > > break;
> > > @@ -836,7 +836,7 @@ int __init xbc_init(char *buf, const char **emsg, int
> > > *epos)
> > > break;
> > > case '#':
> > > q = skip_comment(q);
> > > - /* fall through */
> > > + fallthrough;
> > > case ';':
> > > case '\n':
> > > ret = xbc_parse_key(, q);
> > > diff --git a/lib/cmdline.c b/lib/cmdline.c
> > > index 9e186234edc0..46f2cb4ce6d1 100644
> > > --- a/lib/cmdline.c
> > > +++ b/lib/cmdline.c
> > > @@ -144,23 +144,23 @@ unsigned long long memparse(const char *ptr, char
> > > **retptr)
> > > case 'E':
> > > case 'e':
> > > ret <<= 10;
> > > - /* fall through */
> > > + fallthrough;
> > > case 'P':
> > > case 'p':
> > > ret <<= 10;
> > > - /* fall through */
> > > + fallthrough;
> > > case
Re: [PATCH 2/3] Revert "lib: Revert use of fallthrough pseudo-keyword in lib/"
On Tue, Nov 17, 2020 at 02:28:43PM -0800, Nick Desaulniers wrote: > On Tue, Nov 17, 2020 at 2:16 PM Gustavo A. R. Silva > wrote: > > > > I'm happy to take this series in my tree. I'm planing to send a > > pull-request for -rc5 with more related changes. So, I can include > > this in the same PR. > > > > In the meantime I'll add this to my testing tree, so it can be > > build-tested by the 0-day folks. :) > > SGTM, and thank you. I'm sure you saw the existing warning about > indentation. Do we want to modify the revert patch, or put another > patch on top? In this case, it'd be much better to modify the revert patch. I also saw someone made a comment to the first patch of the series. If you can address both issues and send v2 it'd be great. Anyways, as those are trivial changes, I already added the series to my testing tree for 0-day build-testing. Thanks -- Gustavo
Re: Radeon NI: GIT kernel with the nislands_smc commit doesn't boot on a Freescale P5040 board and P.A.Semi Nemo board
Hi Christian, On 4/30/21 06:59, Christian Zigotzky wrote: > Hello, > > The Nemo board (A-EON AmigaOne X1000) [1] and the FSL P5040 Cyrus+ board > (A-EON AmigaOne X5000) [2] with installed AMD Radeon HD6970 NI graphics cards > (Cayman > XT) [3] don't boot with the latest git kernel anymore after the commit > "drm/radeon/nislands_smc.h: Replace one-element array with flexible-array > member in > struct NISLANDS_SMC_SWSTATE branch" [4]. This git kernel boots in a virtual > e5500 QEMU machine with a VirtIO-GPU [5]. > > I bisected today [6]. > > Result: drm/radeon/nislands_smc.h: Replace one-element array with > flexible-array member in struct NISLANDS_SMC_SWSTATE branch > (434fb1e7444a2efc3a4ebd950c7f771ebfcffa31) [4] is the first bad commit. I have a fix ready for this bug: https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/drm-nislands I wonder if you could help me to test it with your environment, please. It should be applied on top of mainline. Thank you! -- Gustavo > > I was able to revert this commit [7] and after a new compiling, the kernel > boots without any problems on my AmigaOnes. > > After that I created a patch for reverting this commit for new git test > kernels. [3] > > The kernel compiles and boots with this patch on my AmigaOnes. Please find > attached the kernel config files. > > Please check the first bad commit. > > Thanks, > Christian > > [1] https://en.wikipedia.org/wiki/AmigaOne_X1000 > [2] http://wiki.amiga.org/index.php?title=X5000 > [3] https://forum.hyperion-entertainment.com/viewtopic.php?f=35=4377 > [4] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=434fb1e7444a2efc3a4ebd950c7f771ebfcffa31 > [5] qemu-system-ppc64 -M ppce500 -cpu e5500 -m 1024 -kernel uImage -drive > format=raw,file=MintPPC32-X5000.img,index=0,if=virtio -netdev user,id=mynet0 > -device > virtio-net-pci,netdev=mynet0 -append "rw root=/dev/vda" -device virtio-vga > -usb -device usb-ehci,id=ehci -device usb-tablet -device virtio-keyboard-pci > -smp 4 > -vnc :1 > [6] https://forum.hyperion-entertainment.com/viewtopic.php?p=53074#p53074 > [7] git revert 434fb1e7444a2efc3a4ebd950c7f771ebfcffa3
Re: Radeon NI: GIT kernel with the nislands_smc commit doesn't boot on a Freescale P5040 board and P.A.Semi Nemo board
Hi Christian, On 5/8/21 06:33, Christian Zigotzky wrote: > Hi Gustavo, > > Your patch works! Thanks a lot! I tested it with my Freescale P5040 board and > P.A.Semi Nemo board with a connected AMD Radeon HD6970 NI graphics cards > (Cayman > XT) today. Awesome! :) Thank you! -- Gustavo > > Have a nice day, > Christian > > > On 07 May 2021 at 08:43am, Christian Zigotzky wrote: >> Hi Gustavo, >> >> Great! I will test it. Many thanks for your help. >> >> Cheers, >> Christian >> >> >>> On 7. May 2021, at 01:55, Gustavo A. R. Silva >>> wrote: >>> >>> Hi Christian, >>> >>>> On 4/30/21 06:59, Christian Zigotzky wrote: >>>> Hello, >>>> >>>> The Nemo board (A-EON AmigaOne X1000) [1] and the FSL P5040 Cyrus+ board >>>> (A-EON AmigaOne X5000) [2] with installed AMD Radeon HD6970 NI graphics >>>> cards (Cayman >>>> XT) [3] don't boot with the latest git kernel anymore after the commit >>>> "drm/radeon/nislands_smc.h: Replace one-element array with flexible-array >>>> member in >>>> struct NISLANDS_SMC_SWSTATE branch" [4]. This git kernel boots in a >>>> virtual e5500 QEMU machine with a VirtIO-GPU [5]. >>>> >>>> I bisected today [6]. >>>> >>>> Result: drm/radeon/nislands_smc.h: Replace one-element array with >>>> flexible-array member in struct NISLANDS_SMC_SWSTATE branch >>>> (434fb1e7444a2efc3a4ebd950c7f771ebfcffa31) [4] is the first bad commit. >>> I have a fix ready for this bug: >>> https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/drm-nislands >>> >>> I wonder if you could help me to test it with your environment, please. >>> It should be applied on top of mainline. >>> >>> Thank you! >>> -- >>> Gustavo >>> >>>> I was able to revert this commit [7] and after a new compiling, the kernel >>>> boots without any problems on my AmigaOnes. >>>> >>>> After that I created a patch for reverting this commit for new git test >>>> kernels. [3] >>>> >>>> The kernel compiles and boots with this patch on my AmigaOnes. Please find >>>> attached the kernel config files. >>>> >>>> Please check the first bad commit. >>>> >>>> Thanks, >>>> Christian >>>> >>>> [1] https://en.wikipedia.org/wiki/AmigaOne_X1000 >>>> [2] http://wiki.amiga.org/index.php?title=X5000 >>>> [3] https://forum.hyperion-entertainment.com/viewtopic.php?f=35=4377 >>>> [4] >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=434fb1e7444a2efc3a4ebd950c7f771ebfcffa31 >>>> [5] qemu-system-ppc64 -M ppce500 -cpu e5500 -m 1024 -kernel uImage -drive >>>> format=raw,file=MintPPC32-X5000.img,index=0,if=virtio -netdev >>>> user,id=mynet0 -device >>>> virtio-net-pci,netdev=mynet0 -append "rw root=/dev/vda" -device virtio-vga >>>> -usb -device usb-ehci,id=ehci -device usb-tablet -device >>>> virtio-keyboard-pci -smp 4 >>>> -vnc :1 >>>> [6] https://forum.hyperion-entertainment.com/viewtopic.php?p=53074#p53074 >>>> [7] git revert 434fb1e7444a2efc3a4ebd950c7f771ebfcffa3 >
Re: Radeon NI: GIT kernel with the nislands_smc commit doesn't boot on a Freescale P5040 board and P.A.Semi Nemo board
On 4/30/21 10:26, Deucher, Alexander wrote: > [AMD Public Use] > > + Gustavo, amd-gfx > >> -Original Message- >> From: Christian Zigotzky >> Sent: Friday, April 30, 2021 8:00 AM >> To: gustavo...@kernel.org; Deucher, Alexander >> >> Cc: R.T.Dickinson ; Darren Stevens > zone.net>; mad skateman ; linuxppc-dev >> ; Olof Johansson ; >> Maling list - DRI developers ; Michel >> Dänzer ; Christian Zigotzky >> Subject: Radeon NI: GIT kernel with the nislands_smc commit doesn't >> boot on a Freescale P5040 board and P.A.Semi Nemo board >> >> Hello, >> >> The Nemo board (A-EON AmigaOne X1000) [1] and the FSL P5040 Cyrus+ >> board (A-EON AmigaOne X5000) [2] with installed AMD Radeon HD6970 NI >> graphics cards (Cayman XT) [3] don't boot with the latest git kernel >> anymore after the commit "drm/radeon/nislands_smc.h: Replace >> one-element array with flexible-array member in struct NISLANDS_SMC_SWSTATE >> branch" [4]. >> This git kernel boots in a virtual e5500 QEMU machine with a VirtIO-GPU [5]. >> >> I bisected today [6]. >> >> Result: drm/radeon/nislands_smc.h: Replace one-element array with >> flexible-array member in struct NISLANDS_SMC_SWSTATE branch >> (434fb1e7444a2efc3a4ebd950c7f771ebfcffa31) [4] is the first bad commit. >> >> I was able to revert this commit [7] and after a new compiling, the >> kernel boots without any problems on my AmigaOnes. >> >> After that I created a patch for reverting this commit for new git test >> kernels. >> [3] >> >> The kernel compiles and boots with this patch on my AmigaOnes. Please >> find attached the kernel config files. >> >> Please check the first bad commit. I'll have a look. Thanks for the report! -- Gustavo >> >> Thanks, >> Christian >> >> [1] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen. >> wikipedia.org%2Fwiki%2FAmigaOne_X1000data=04%7C01%7Calexand >> er.deucher%40amd.com%7C0622549383fb4320346b08d90bcf7be1%7C3dd89 >> 61fe4884e608e11a82d994e183d%7C0%7C0%7C637553808670161651%7CUnkn >> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik >> 1haWwiLCJXVCI6Mn0%3D%7C1000sdata=PNSrApUdMrku20hH7dEKlJJ >> TBi7Qp5JOkqpA4MvKqdE%3Dreserved=0 >> [2] >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwiki. >> a miga.org%2Findex.php%3Ftitle%3DX5000data=04%7C01%7Calexander >> .deucher%40amd.com%7C0622549383fb4320346b08d90bcf7be1%7C3dd8961f >> e4884e608e11a82d994e183d%7C0%7C0%7C637553808670161651%7CUnknow >> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha >> WwiLCJXVCI6Mn0%3D%7C1000sdata=B8Uvhs25%2FP3RfnL1AgICN3Y4 >> CEXeCE1yIoi3vvwvGto%3Dreserved=0 >> [3] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforu >> m.hyperion- >> entertainment.com%2Fviewtopic.php%3Ff%3D35%26t%3D4377data= >> 04%7C01%7Calexander.deucher%40amd.com%7C0622549383fb4320346b08d >> 90bcf7be1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63755380 >> 8670161651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj >> oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=TokXplD >> Tvg3%2BZMPLCgR1fs%2BN2X9MIfLXLW67MAM2Qsk%3Dreserved=0 >> [4] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit. >> k ernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git% >> 2Fcommit%2F%3Fid%3D434fb1e7444a2efc3a4ebd950c7f771ebfcffa31d >> ata=04%7C01%7Calexander.deucher%40amd.com%7C0622549383fb4320346 >> b08d90bcf7be1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6375 >> 53808670161651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL >> CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=JC >> M4xvPEnWdckcTPbQ2Ujv%2FAiMMsFMzzl4Pr%2FRPlcMQ%3Dreserve >> d=0 >> [5] qemu-system-ppc64 -M ppce500 -cpu e5500 -m 1024 -kernel uImage - >> drive format=raw,file=MintPPC32-X5000.img,index=0,if=virtio -netdev >> user,id=mynet0 -device virtio-net-pci,netdev=mynet0 -append "rw >> root=/dev/vda" -device virtio-vga -usb -device usb-ehci,id=ehci >> -device usb- tablet -device virtio-keyboard-pci -smp 4 -vnc :1 [6] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforu >> m.hyperion- >> entertainment.com%2Fviewtopic.php%3Fp%3D53074%23p53074data >> =04%7C01%7Calexander.deucher%40amd.com%7C0622549383fb4320346b08 >> d90bcf7be1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6375538 >> 08670161651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ >> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=RXfSlY >> A3bDEFas0%2Fk2vMWsl2l0nuhS2ecjZgSBLc%2Bs4%3Dreserved=0 >> [7] git revert 434fb1e7444a2efc3a4ebd950c7f771ebfcffa3
Re: [PATCH] KVM: PPC: Replace zero-length array with flexible array member
On Mon, Sep 20, 2021 at 07:05:07PM -0500, Gustavo A. R. Silva wrote: > > > On 9/18/21 09:21, Len Baker wrote: > > There is a regular need in the kernel to provide a way to declare having > > a dynamically sized set of trailing elements in a structure. Kernel code > > should always use "flexible array members" [1] for these cases. The > > older style of one-element or zero-length arrays should no longer be > > used[2]. > > > > Also, make use of the struct_size() helper in kzalloc(). > > > > [1] https://en.wikipedia.org/wiki/Flexible_array_member > > [2] > > https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays > > > > Signed-off-by: Len Baker > > Reviewed-by: Gustavo A. R. Silva I'm taking this in my -next tree. Thanks -- Gustavo
Re: [PATCH][next] powerpc/vas: Fix potential NULL pointer dereference
On Mon, Oct 18, 2021 at 02:09:31PM -0700, Tyrel Datwyler wrote:
> On 10/14/21 10:03 PM, Gustavo A. R. Silva wrote:
> > (!ptr && !ptr->foo) strikes again. :)
> >
> > The expression (!ptr && !ptr->foo) is bogus and in case ptr is NULL,
> > it leads to a NULL pointer dereference: ptr->foo.
> >
> > Fix this by converting && to ||
> >
> > This issue was detected with the help of Coccinelle, and audited and
> > fixed manually.
> >
> > Fixes: 1a0d0d5ed5e3 ("powerpc/vas: Add platform specific user window
> > operations")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Gustavo A. R. Silva
> Looking at the usage pattern it is obvious that if we determine !ptr
> attempting
> to also confirm !ptr->ops is going to blow up.
>
> LGTM.
>
> Reviewed-by: Tyrel Datwyler
Thanks, Tyrel.
--
Gustavo
>
> > ---
> > arch/powerpc/platforms/book3s/vas-api.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/book3s/vas-api.c
> > b/arch/powerpc/platforms/book3s/vas-api.c
> > index 30172e52e16b..4d82c92ddd52 100644
> > --- a/arch/powerpc/platforms/book3s/vas-api.c
> > +++ b/arch/powerpc/platforms/book3s/vas-api.c
> > @@ -303,7 +303,7 @@ static int coproc_ioc_tx_win_open(struct file *fp,
> > unsigned long arg)
> > return -EINVAL;
> > }
> >
> > - if (!cp_inst->coproc->vops && !cp_inst->coproc->vops->open_win) {
> > + if (!cp_inst->coproc->vops || !cp_inst->coproc->vops->open_win) {
> > pr_err("VAS API is not registered\n");
> > return -EACCES;
> > }
> > @@ -373,7 +373,7 @@ static int coproc_mmap(struct file *fp, struct
> > vm_area_struct *vma)
> > return -EINVAL;
> > }
> >
> > - if (!cp_inst->coproc->vops && !cp_inst->coproc->vops->paste_addr) {
> > + if (!cp_inst->coproc->vops || !cp_inst->coproc->vops->paste_addr) {
> > pr_err("%s(): VAS API is not registered\n", __func__);
> > return -EACCES;
> > }
> >
>
[PATCH][next] powerpc/vas: Fix potential NULL pointer dereference
(!ptr && !ptr->foo) strikes again. :)
The expression (!ptr && !ptr->foo) is bogus and in case ptr is NULL,
it leads to a NULL pointer dereference: ptr->foo.
Fix this by converting && to ||
This issue was detected with the help of Coccinelle, and audited and
fixed manually.
Fixes: 1a0d0d5ed5e3 ("powerpc/vas: Add platform specific user window
operations")
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/platforms/book3s/vas-api.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/book3s/vas-api.c
b/arch/powerpc/platforms/book3s/vas-api.c
index 30172e52e16b..4d82c92ddd52 100644
--- a/arch/powerpc/platforms/book3s/vas-api.c
+++ b/arch/powerpc/platforms/book3s/vas-api.c
@@ -303,7 +303,7 @@ static int coproc_ioc_tx_win_open(struct file *fp, unsigned
long arg)
return -EINVAL;
}
- if (!cp_inst->coproc->vops && !cp_inst->coproc->vops->open_win) {
+ if (!cp_inst->coproc->vops || !cp_inst->coproc->vops->open_win) {
pr_err("VAS API is not registered\n");
return -EACCES;
}
@@ -373,7 +373,7 @@ static int coproc_mmap(struct file *fp, struct
vm_area_struct *vma)
return -EINVAL;
}
- if (!cp_inst->coproc->vops && !cp_inst->coproc->vops->paste_addr) {
+ if (!cp_inst->coproc->vops || !cp_inst->coproc->vops->paste_addr) {
pr_err("%s(): VAS API is not registered\n", __func__);
return -EACCES;
}
--
2.27.0
Re: [PATCH] KVM: PPC: Replace zero-length array with flexible array member
On 9/18/21 09:21, Len Baker wrote:
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use "flexible array members" [1] for these cases. The
> older style of one-element or zero-length arrays should no longer be
> used[2].
>
> Also, make use of the struct_size() helper in kzalloc().
>
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2]
> https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays
>
> Signed-off-by: Len Baker
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
> ---
> arch/powerpc/include/asm/kvm_host.h | 2 +-
> arch/powerpc/kvm/book3s_64_vio.c| 3 +--
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_host.h
> b/arch/powerpc/include/asm/kvm_host.h
> index 080a7feb7731..3aed653373a5 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -190,7 +190,7 @@ struct kvmppc_spapr_tce_table {
> u64 size; /* window size in pages */
> struct list_head iommu_tables;
> struct mutex alloc_lock;
> - struct page *pages[0];
> + struct page *pages[];
> };
>
> /* XICS components, defined in book3s_xics.c */
> diff --git a/arch/powerpc/kvm/book3s_64_vio.c
> b/arch/powerpc/kvm/book3s_64_vio.c
> index 6365087f3160..d42b4b6d4a79 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -295,8 +295,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> return ret;
>
> ret = -ENOMEM;
> - stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *),
> - GFP_KERNEL);
> + stt = kzalloc(struct_size(stt, pages, npages), GFP_KERNEL);
> if (!stt)
> goto fail_acct;
>
> --
> 2.25.1
>
Re: [PATCH][next] powerpc/vas: Fix potential NULL pointer dereference
On Mon, Oct 18, 2021 at 02:09:31PM -0700, Tyrel Datwyler wrote:
> On 10/14/21 10:03 PM, Gustavo A. R. Silva wrote:
> > (!ptr && !ptr->foo) strikes again. :)
> >
> > The expression (!ptr && !ptr->foo) is bogus and in case ptr is NULL,
> > it leads to a NULL pointer dereference: ptr->foo.
> >
> > Fix this by converting && to ||
> >
> > This issue was detected with the help of Coccinelle, and audited and
> > fixed manually.
> >
> > Fixes: 1a0d0d5ed5e3 ("powerpc/vas: Add platform specific user window
> > operations")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Gustavo A. R. Silva
> Looking at the usage pattern it is obvious that if we determine !ptr
> attempting
> to also confirm !ptr->ops is going to blow up.
>
> LGTM.
>
> Reviewed-by: Tyrel Datwyler
I think I'll take this in my tree.
Thanks, Tyrel.
--
Gustavo
Re: [PATCH][next] powerpc/vas: Fix potential NULL pointer dereference
On Wed, Oct 27, 2021 at 09:30:53AM +1100, Michael Ellerman wrote: [..] > > I think I'll take this in my tree. > > I've already put it in powerpc/next: > > > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?h=next=61cb9ac66b30374c7fd8a8b2a3c4f8f432c72e36 Oh, great. :) > If you need to pick it up as well for some reason that's fine. I just didn't want it to get lost somehow. I'll drop it from tree now. Thanks -- Gustavo
[PATCH][next] net: spider_net: Use size_add() in call to struct_size()
If, for any reason, the open-coded arithmetic causes a wraparound,
the protection that `struct_size()` adds against potential integer
overflows is defeated. Fix this by hardening call to `struct_size()`
with `size_add()`.
Fixes: 3f1071ec39f7 ("net: spider_net: Use struct_size() helper")
Signed-off-by: Gustavo A. R. Silva
---
drivers/net/ethernet/toshiba/spider_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/toshiba/spider_net.c
b/drivers/net/ethernet/toshiba/spider_net.c
index 50d7eacfec58..87e67121477c 100644
--- a/drivers/net/ethernet/toshiba/spider_net.c
+++ b/drivers/net/ethernet/toshiba/spider_net.c
@@ -2332,7 +2332,7 @@ spider_net_alloc_card(void)
struct spider_net_card *card;
netdev = alloc_etherdev(struct_size(card, darray,
- tx_descriptors + rx_descriptors));
+ size_add(tx_descriptors,
rx_descriptors)));
if (!netdev)
return NULL;
--
2.34.1
[PATCH][next] powerpc: Fix fall-through warning for Clang
Fix the following fallthrough warning: arch/powerpc/platforms/85xx/mpc85xx_cds.c:161:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough] Link: https://github.com/KSPP/linux/issues/198 Reported-by: kernel test robot Link: https://lore.kernel.org/lkml/202209061224.kxorrgvg-...@intel.com/ Signed-off-by: Gustavo A. R. Silva --- arch/powerpc/platforms/85xx/mpc85xx_cds.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/powerpc/platforms/85xx/mpc85xx_cds.c b/arch/powerpc/platforms/85xx/mpc85xx_cds.c index 48f3acfece0b..0b8f2101c5fb 100644 --- a/arch/powerpc/platforms/85xx/mpc85xx_cds.c +++ b/arch/powerpc/platforms/85xx/mpc85xx_cds.c @@ -159,6 +159,7 @@ static void __init mpc85xx_cds_pci_irq_fixup(struct pci_dev *dev) else dev->irq = 10; pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq); + break; default: break; } -- 2.34.1
[PATCH][next] powerpc/xmon: Fix -Wswitch-unreachable warning in bpt_cmds
When building with automatic stack variable initialization, GCC 12
complains about variables defined outside of switch case statements.
Move the variable into the case that uses it, which silences the warning:
arch/powerpc/xmon/xmon.c: In function ‘bpt_cmds’:
arch/powerpc/xmon/xmon.c:1529:13: warning: statement will never be executed
[-Wswitch-unreachable]
1529 | int mode;
| ^~~~
Fixes: 09b6c1129f89 ("powerpc/xmon: Fix compile error with PPC_8xx=y")
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/xmon/xmon.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 26ef3388c24c..df91dfc7ff72 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1525,9 +1525,9 @@ bpt_cmds(void)
cmd = inchar();
switch (cmd) {
- static const char badaddr[] = "Only kernel addresses are permitted for
breakpoints\n";
- int mode;
- case 'd': /* bd - hardware data breakpoint */
+ case 'd': { /* bd - hardware data breakpoint */
+ static const char badaddr[] = "Only kernel addresses are
permitted for breakpoints\n";
+ int mode;
if (xmon_is_ro) {
printf(xmon_ro_msg);
break;
@@ -1560,6 +1560,7 @@ bpt_cmds(void)
force_enable_xmon();
break;
+ }
case 'i': /* bi - hardware instr breakpoint */
if (xmon_is_ro) {
--
2.34.1
Re: [PATCH][next] powerpc: Fix fall-through warning for Clang
Applied to powerpc/next. Great. :) Thanks, Michael. -- Gustavo
Re: [PATCH] powerpc/lib: Avoid array bounds warnings in vec ops
To be honest I don't know how paranoid we want to get, we could end up putting WARN's all over the kernel :) In this case I guess if the size is too large we overflow the buffer on the kernel stack, so we should at least check the size. But does it need a WARN? I'm not sure. If we had a case that was passing a out-of-bound size hopefully we would notice in testing? :) You're right, a simpler check should suffice. I will send an updated patch. This[1] patch indeed also makes those -Wstringop-overflow warnings go away. :) I'm not subscribed to the list but here are my Reviewed-by: Gustavo A. R. Silva Build-tested-by: Gustavo A. R. Silva Thank you guys! -- Gustavo [1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-November/265936.html
Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
On 11/20/23 08:25, Naveen N Rao wrote:
On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
Hi all,
I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
to get your feedback on this, please:
In function 'do_byte_reverse',
inlined from 'do_vec_store' at
/home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~^
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination
object 'u' of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_store' at
/home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
289 | up[2] = byterev_8(up[1]);
| ~~^~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u'
of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_load' at
/home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~^
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
The origing of the issue seems to be the following calls to function
`do_vec_store()`, when
`size > 16`, or more precisely when `size == 32`
arch/powerpc/lib/sstep.c:
3436 case LOAD_VMX:
3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3438 return 0;
3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
3440 break;
...
3507 case STORE_VMX:
3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3509 return 0;
3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
3511 break;
LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
exceed 16. So, the warning looks incorrect to me.
Does that mean that this piece of code is never actually executed:
281 case 32: {
282 unsigned long *up = (unsigned long *)ptr;
283 unsigned long tmp;
284
285 tmp = byterev_8(up[0]);
286 up[0] = byterev_8(up[3]);
287 up[3] = tmp;
288 tmp = byterev_8(up[2]);
289 up[2] = byterev_8(up[1]);
290 up[1] = tmp;
291 break;
292 }
or rather, under which conditions the above code is executed, if any?
Thanks!
--
Gustavo
[RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
Hi all,
I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
to get your feedback on this, please:
In function 'do_byte_reverse',
inlined from 'do_vec_store' at
/home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~^
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination
object 'u' of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_store' at
/home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
289 | up[2] = byterev_8(up[1]);
| ~~^~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u'
of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_load' at
/home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~^
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
The origing of the issue seems to be the following calls to function
`do_vec_store()`, when
`size > 16`, or more precisely when `size == 32`
arch/powerpc/lib/sstep.c:
3436 case LOAD_VMX:
3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3438 return 0;
3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
3440 break;
...
3507 case STORE_VMX:
3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3509 return 0;
3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
3511 break;
Inside `do_vec_store()`, we have that the size of `union u` is 16 bytes:
702 int size, struct pt_regs *regs,
703 bool cross_endian)
704 {
705 union {
706 __vector128 v;
707 u8 b[sizeof(__vector128)];
708 } u;
and then function `do_byte_reverse()` is called
721 if (unlikely(cross_endian))
722 do_byte_reverse([ea & 0xf], size);
and if `size == 32`, the following piece of code tries to access
`ptr` outside of its boundaries:
260 static nokprobe_inline void do_byte_reverse(void *ptr, int nb)
261 {
262 switch (nb) {
...
281 case 32: {
282 unsigned long *up = (unsigned long *)ptr;
283 unsigned long tmp;
284
285 tmp = byterev_8(up[0]);
286 up[0] = byterev_8(up[3]);
287 up[3] = tmp;
288 tmp = byterev_8(up[2]);
289 up[2] = byterev_8(up[1]);
290 up[1] = tmp;
291 break;
292 }
The following patch is merely a test to illustrate how the value in `size`
initially appears
to influence the warnings:
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index a4ab8625061a..86786957b736 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -3436,7 +3436,7 @@ int emulate_loadstore(struct pt_regs *regs, struct
instruction_op *op)
case LOAD_VMX:
if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
return 0;
- err = do_vec_load(op->reg, ea, size, regs, cross_endian);
+ err = do_vec_load(op->reg, ea, (size > 16) ? 16 : size, regs,
cross_endian);
break;
#endif
#ifdef CONFIG_VSX
@@ -3507,7 +3507,7 @@ int emulate_loadstore(struct pt_regs *regs, struct
instruction_op *op)
case STORE_VMX:
if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
return 0;
- err = do_vec_store(op->reg, ea, size, regs, cross_endian);
+ err = do_vec_store(op->reg, ea, (size > 16) ? 16 : size,
Re: [PATCH] powerpc/lib: Avoid array bounds warnings in vec ops
On 11/20/23 17:54, Michael Ellerman wrote:
Building with GCC 13 (which has -array-bounds enabled) there are several
warnings in sstep.c along the lines of:
In function ‘do_byte_reverse’,
inlined from ‘do_vec_load’ at arch/powerpc/lib/sstep.c:691:3,
inlined from ‘emulate_loadstore’ at arch/powerpc/lib/sstep.c:3439:9:
arch/powerpc/lib/sstep.c:289:23: error: array subscript 2 is outside array
bounds of ‘u8[16]’ {aka ‘unsigned char[16]’} [-Werror=array-bounds=]
289 | up[2] = byterev_8(up[1]);
| ~~^~
arch/powerpc/lib/sstep.c: In function ‘emulate_loadstore’:
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object ‘u’ of size
16
681 | } u = {};
| ^
do_byte_reverse() supports a size up to 32 bytes, but in these cases the
caller is only passing a 16 byte buffer. In practice there is no bug,
do_vec_load() is only called from the LOAD_VMX case in emulate_loadstore().
That in turn is only reached when analyse_instr() recognises VMX ops,
and in all cases the size is no greater than 16:
$ git grep -w LOAD_VMX arch/powerpc/lib/sstep.c
arch/powerpc/lib/sstep.c:op->type = MKOP(LOAD_VMX,
0, 1);
arch/powerpc/lib/sstep.c:op->type = MKOP(LOAD_VMX,
0, 2);
arch/powerpc/lib/sstep.c:op->type = MKOP(LOAD_VMX,
0, 4);
arch/powerpc/lib/sstep.c:op->type = MKOP(LOAD_VMX,
0, 16);
Similarly for do_vec_store().
Although the warning is incorrect, the code would be safer if it clamped
the size from the caller to the known size of the buffer. Do that using
min_t().
Reported-by: Bagas Sanjaya
Reported-by: Jan-Benedict Glaw
Reported-by: Gustavo A. R. Silva
Signed-off-by: Michael Ellerman
Reviewed-by: Gustavo A. R. Silva
Build-tested-by: Gustavo A. R. Silva
This indeed makes all those warnings go away. :)
Thanks, Michael!
--
Gustavo
---
arch/powerpc/lib/sstep.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index a4ab8625061a..a13f05cfc7db 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -688,7 +688,7 @@ static nokprobe_inline int do_vec_load(int rn, unsigned
long ea,
if (err)
return err;
if (unlikely(cross_endian))
- do_byte_reverse([ea & 0xf], size);
+ do_byte_reverse([ea & 0xf], min_t(size_t, size, sizeof(u)));
preempt_disable();
if (regs->msr & MSR_VEC)
put_vr(rn, );
@@ -719,7 +719,7 @@ static nokprobe_inline int do_vec_store(int rn, unsigned
long ea,
u.v = current->thread.vr_state.vr[rn];
preempt_enable();
if (unlikely(cross_endian))
- do_byte_reverse([ea & 0xf], size);
+ do_byte_reverse([ea & 0xf], min_t(size_t, size, sizeof(u)));
return copy_mem_out([ea & 0xf], ea, size, regs);
}
#endif /* CONFIG_ALTIVEC */
Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
On 11/20/23 09:21, Naveen N Rao wrote:
On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote:
On 11/20/23 08:25, Naveen N Rao wrote:
On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
Hi all,
I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
to get your feedback on this, please:
In function 'do_byte_reverse',
inlined from 'do_vec_store' at
/home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~^
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination
object 'u' of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_store' at
/home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
289 | up[2] = byterev_8(up[1]);
| ~~^~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u'
of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_load' at
/home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at
/home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0
[-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~^
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination
object 'u' of size 16
The origing of the issue seems to be the following calls to function
`do_vec_store()`, when
`size > 16`, or more precisely when `size == 32`
arch/powerpc/lib/sstep.c:
3436 case LOAD_VMX:
3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3438 return 0;
3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
3440 break;
...
3507 case STORE_VMX:
3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3509 return 0;
3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
3511 break;
LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
exceed 16. So, the warning looks incorrect to me.
Does that mean that this piece of code is never actually executed:
281 case 32: {
282 unsigned long *up = (unsigned long *)ptr;
283 unsigned long tmp;
284
285 tmp = byterev_8(up[0]);
286 up[0] = byterev_8(up[3]);
287 up[3] = tmp;
288 tmp = byterev_8(up[2]);
289 up[2] = byterev_8(up[1]);
290 up[1] = tmp;
291 break;
292 }
or rather, under which conditions the above code is executed, if any?
Please see git history to understand the commit that introduced that
code. You can do that by starting with a 'git blame' on the file. You'll
find that the commit that introduced the above hunk was af99da74333b
("powerpc/sstep: Support VSX vector paired storage access
instructions").
The above code path is hit via
do_vsx_load()->emulate_vsx_load()->do_byte_reverse()
It seems there is another path (at least the one that triggers the
-Wstringop-overflow warnings):
emulate_step()->emulate_loadstore()->do_vec_load()->do_byte_reverse()
emulate_step() {
...
if (OP_IS_LOAD_STORE(type)) {
err = emulate_loadstore(regs, );
if (err)
return 0;
goto instr_done;
}
...
}
> emulate_loadstore() {
...
#ifdef CONFIG_ALTIVEC
case LOAD_VMX:
if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
return 0;
err = do_vec_load(op->reg, ea, size, regs, cross_endian); //
with size == 32
break;
#endif
...
#ifdef CONFIG_ALTIVEC
case STORE_VMX:
if (!(regs->msr & MSR_PR)
[PATCH][next] powerpc/crypto: Avoid -Wstringop-overflow warnings
The compiler doesn't know that `32` is an offset into the Hash table:
56 struct Hash_ctx {
57 u8 H[16]; /* subkey */
58 u8 Htable[256]; /* Xi, Hash table(offset 32) */
59 };
So, it legitimately complains about a potential out-of-bounds issue
if `256 bytes` are accessed in `htable` (this implies going
`32 bytes` beyond the boundaries of `Htable`):
arch/powerpc/crypto/aes-gcm-p10-glue.c: In function 'gcmp10_init':
arch/powerpc/crypto/aes-gcm-p10-glue.c:120:9: error: 'gcm_init_htable'
accessing 256 bytes in a region of size 224 [-Werror=stringop-overflow=]
120 | gcm_init_htable(hash->Htable+32, hash->H);
| ^
arch/powerpc/crypto/aes-gcm-p10-glue.c:120:9: note: referencing argument 1 of
type 'unsigned char[256]'
arch/powerpc/crypto/aes-gcm-p10-glue.c:120:9: note: referencing argument 2 of
type 'unsigned char[16]'
arch/powerpc/crypto/aes-gcm-p10-glue.c:40:17: note: in a call to function
'gcm_init_htable'
40 | asmlinkage void gcm_init_htable(unsigned char htable[256], unsigned
char Xi[16]);
| ^~~
Address this by avoiding specifying the size of `htable` in the function
prototype; and just for consistency, do the same for parameter `Xi`.
Reported-by: Stephen Rothwell
Closes:
https://lore.kernel.org/linux-next/20231121131903.68a37...@canb.auug.org.au/
Signed-off-by: Gustavo A. R. Silva
---
arch/powerpc/crypto/aes-gcm-p10-glue.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/crypto/aes-gcm-p10-glue.c
b/arch/powerpc/crypto/aes-gcm-p10-glue.c
index 4b6e899895e7..f62ee54076c0 100644
--- a/arch/powerpc/crypto/aes-gcm-p10-glue.c
+++ b/arch/powerpc/crypto/aes-gcm-p10-glue.c
@@ -37,7 +37,7 @@ asmlinkage void aes_p10_gcm_encrypt(u8 *in, u8 *out, size_t
len,
void *rkey, u8 *iv, void *Xi);
asmlinkage void aes_p10_gcm_decrypt(u8 *in, u8 *out, size_t len,
void *rkey, u8 *iv, void *Xi);
-asmlinkage void gcm_init_htable(unsigned char htable[256], unsigned char
Xi[16]);
+asmlinkage void gcm_init_htable(unsigned char htable[], unsigned char Xi[]);
asmlinkage void gcm_ghash_p10(unsigned char *Xi, unsigned char *Htable,
unsigned char *aad, unsigned int alen);
--
2.34.1
[PATCH][next] crypto/nx: Avoid -Wflex-array-member-not-at-end warning
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally. So, we are deprecating flexible-array
members in the middle of another structure.
There is currently an object (`header`) in `struct nx842_crypto_ctx`
that contains a flexible structure (`struct nx842_crypto_header`):
struct nx842_crypto_ctx {
...
struct nx842_crypto_header header;
struct nx842_crypto_header_group group[NX842_CRYPTO_GROUP_MAX];
...
};
So, in order to avoid ending up with a flexible-array member in the
middle of another struct, we use the `struct_group_tagged()` helper to
separate the flexible array from the rest of the members in the flexible
structure:
struct nx842_crypto_header {
struct_group_tagged(nx842_crypto_header_hdr, hdr,
... the rest of the members
);
struct nx842_crypto_header_group group[];
} __packed;
With the change described above, we can now declare an object of the
type of the tagged struct, without embedding the flexible array in the
middle of another struct:
struct nx842_crypto_ctx {
...
struct nx842_crypto_header_hdr header;
struct nx842_crypto_header_group group[NX842_CRYPTO_GROUP_MAX];
...
} __packed;
We also use `container_of()` whenever we need to retrieve a pointer to
the flexible structure, through which we can access the flexible
array if needed.
So, with these changes, fix the following warning:
In file included from drivers/crypto/nx/nx-842.c:55:
drivers/crypto/nx/nx-842.h:174:36: warning: structure containing a flexible
array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
174 | struct nx842_crypto_header header;
|^~
Signed-off-by: Gustavo A. R. Silva
---
drivers/crypto/nx/nx-842.c | 6 --
drivers/crypto/nx/nx-842.h | 10 ++
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c
index 2ab90ec10e61..82214cde2bcd 100644
--- a/drivers/crypto/nx/nx-842.c
+++ b/drivers/crypto/nx/nx-842.c
@@ -251,7 +251,9 @@ int nx842_crypto_compress(struct crypto_tfm *tfm,
u8 *dst, unsigned int *dlen)
{
struct nx842_crypto_ctx *ctx = crypto_tfm_ctx(tfm);
- struct nx842_crypto_header *hdr = >header;
+ struct nx842_crypto_header *hdr =
+ container_of(>header,
+struct nx842_crypto_header, hdr);
struct nx842_crypto_param p;
struct nx842_constraints c = *ctx->driver->constraints;
unsigned int groups, hdrsize, h;
@@ -490,7 +492,7 @@ int nx842_crypto_decompress(struct crypto_tfm *tfm,
}
memcpy(>header, src, hdr_len);
- hdr = >header;
+ hdr = container_of(>header, struct nx842_crypto_header, hdr);
for (n = 0; n < hdr->groups; n++) {
/* ignore applies to last group */
diff --git a/drivers/crypto/nx/nx-842.h b/drivers/crypto/nx/nx-842.h
index 7590bfb24d79..25fa70b2112c 100644
--- a/drivers/crypto/nx/nx-842.h
+++ b/drivers/crypto/nx/nx-842.h
@@ -157,9 +157,11 @@ struct nx842_crypto_header_group {
} __packed;
struct nx842_crypto_header {
- __be16 magic; /* NX842_CRYPTO_MAGIC */
- __be16 ignore; /* decompressed end bytes to ignore */
- u8 groups; /* total groups in this header */
+ struct_group_tagged(nx842_crypto_header_hdr, hdr,
+ __be16 magic; /* NX842_CRYPTO_MAGIC */
+ __be16 ignore; /* decompressed end bytes to ignore */
+ u8 groups; /* total groups in this header */
+ );
struct nx842_crypto_header_group group[];
} __packed;
@@ -171,7 +173,7 @@ struct nx842_crypto_ctx {
u8 *wmem;
u8 *sbounce, *dbounce;
- struct nx842_crypto_header header;
+ struct nx842_crypto_header_hdr header;
struct nx842_crypto_header_group group[NX842_CRYPTO_GROUP_MAX];
struct nx842_driver *driver;
--
2.34.1
[PATCH][next] crypto/nx: Avoid potential -Wflex-array-member-not-at-end warning
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.
Use the `__struct_group()` helper to separate the flexible array
from the rest of the members in flexible `struct nx842_crypto_header`,
through tagged `struct nx842_crypto_header_hdr`, and avoid embedding
the flexible-array member in the middle of `struct nx842_crypto_ctx`.
Also, use `container_of()` whenever we need to retrieve a pointer to
the flexible structure.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link: https://github.com/KSPP/linux/issues/202
Signed-off-by: Gustavo A. R. Silva
---
drivers/crypto/nx/nx-842.c | 6 --
drivers/crypto/nx/nx-842.h | 11 +++
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c
index 2ab90ec10e61..82214cde2bcd 100644
--- a/drivers/crypto/nx/nx-842.c
+++ b/drivers/crypto/nx/nx-842.c
@@ -251,7 +251,9 @@ int nx842_crypto_compress(struct crypto_tfm *tfm,
u8 *dst, unsigned int *dlen)
{
struct nx842_crypto_ctx *ctx = crypto_tfm_ctx(tfm);
- struct nx842_crypto_header *hdr = >header;
+ struct nx842_crypto_header *hdr =
+ container_of(>header,
+struct nx842_crypto_header, hdr);
struct nx842_crypto_param p;
struct nx842_constraints c = *ctx->driver->constraints;
unsigned int groups, hdrsize, h;
@@ -490,7 +492,7 @@ int nx842_crypto_decompress(struct crypto_tfm *tfm,
}
memcpy(>header, src, hdr_len);
- hdr = >header;
+ hdr = container_of(>header, struct nx842_crypto_header, hdr);
for (n = 0; n < hdr->groups; n++) {
/* ignore applies to last group */
diff --git a/drivers/crypto/nx/nx-842.h b/drivers/crypto/nx/nx-842.h
index 7590bfb24d79..1f42c83d2683 100644
--- a/drivers/crypto/nx/nx-842.h
+++ b/drivers/crypto/nx/nx-842.h
@@ -157,9 +157,12 @@ struct nx842_crypto_header_group {
} __packed;
struct nx842_crypto_header {
- __be16 magic; /* NX842_CRYPTO_MAGIC */
- __be16 ignore; /* decompressed end bytes to ignore */
- u8 groups; /* total groups in this header */
+ /* New members must be added within the __struct_group() macro below. */
+ __struct_group(nx842_crypto_header_hdr, hdr, __packed,
+ __be16 magic; /* NX842_CRYPTO_MAGIC */
+ __be16 ignore; /* decompressed end bytes to ignore */
+ u8 groups; /* total groups in this header */
+ );
struct nx842_crypto_header_group group[];
} __packed;
@@ -171,7 +174,7 @@ struct nx842_crypto_ctx {
u8 *wmem;
u8 *sbounce, *dbounce;
- struct nx842_crypto_header header;
+ struct nx842_crypto_header_hdr header;
struct nx842_crypto_header_group group[NX842_CRYPTO_GROUP_MAX];
struct nx842_driver *driver;
--
2.34.1
