LinuxLists.cc - [PATCH 00/11] treewide: mask then shift defects and style updates

2014-10-27 05:25:24

Subject: [PATCH 00/11] treewide: mask then shift defects and style updates

logical mask has lower precedence than shift but should be
done before the shift so parentheses are generally required.

And when masking with a fixed value after a shift, normal kernel
style has the shift on the left, then the shift on the right so
convert a few non-conforming uses.

Joe Perches (11):
block: nvme-scsi: Fix probable mask then right shift defects
radeon: evergreen: Fix probable mask then right shift defects
aiptek: Fix probable mask then right shift defects
dvb-net: Fix probable mask then right shift defects
cx25840/cx18: Use standard ordering of mask and shift
wm8350-core: Fix probable mask then right shift defect
iwlwifi: dvm: Fix probable mask then right shift defect
ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect
tty: ipwireless: Fix probable mask then right shift defects
hwa-hc: Fix probable mask then right shift defect
sound: ad1889: Fix probable mask then right shift defects

drivers/block/nvme-scsi.c | 12 ++++++------
drivers/gpu/drm/radeon/evergreen.c | 3 ++-
drivers/input/tablet/aiptek.c | 6 +++---
drivers/media/dvb-core/dvb_net.c | 4 +++-
drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
drivers/media/pci/cx18/cx18-av-core.c | 16 ++++++++--------
drivers/mfd/wm8350-core.c | 2 +-
drivers/net/wireless/iwlwifi/dvm/lib.c | 4 ++--
drivers/ssb/driver_chipcommon_pmu.c | 4 ++--
drivers/tty/ipwireless/hardware.c | 12 ++++++------
drivers/usb/host/hwa-hc.c | 2 +-
sound/pci/ad1889.c | 8 ++++----
12 files changed, 44 insertions(+), 41 deletions(-)

--
2.1.2

2014-10-27 05:25:26

by Joe Perches

[permalink] [raw]

Subject: [PATCH 01/11] block: nvme-scsi: Fix probable mask then right shift defects

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by: Joe Perches <[email protected]>
---
drivers/block/nvme-scsi.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nvme-scsi.c b/drivers/block/nvme-scsi.c
index a4cd6d6..529ee54 100644
--- a/drivers/block/nvme-scsi.c
+++ b/drivers/block/nvme-scsi.c
@@ -1972,8 +1972,8 @@ static inline void nvme_trans_get_io_cdb10(u8 *cmd,
{
cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_10_CDB_FUA_OFFSET) &
IO_CDB_FUA_MASK;
- cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
- IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+ cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
+ IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_10_CDB_LBA_OFFSET);
cdb_info->xfer_len = GET_U16_FROM_CDB(cmd, IO_10_CDB_TX_LEN_OFFSET);
}
@@ -1983,8 +1983,8 @@ static inline void nvme_trans_get_io_cdb12(u8 *cmd,
{
cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_12_CDB_FUA_OFFSET) &
IO_CDB_FUA_MASK;
- cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
- IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+ cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
+ IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_12_CDB_LBA_OFFSET);
cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_12_CDB_TX_LEN_OFFSET);
}
@@ -1994,8 +1994,8 @@ static inline void nvme_trans_get_io_cdb16(u8 *cmd,
{
cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_16_CDB_FUA_OFFSET) &
IO_CDB_FUA_MASK;
- cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
- IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
+ cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
+ IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
cdb_info->lba = GET_U64_FROM_CDB(cmd, IO_16_CDB_LBA_OFFSET);
cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_16_CDB_TX_LEN_OFFSET);
}
--
2.1.2

2014-10-27 05:25:28

by Joe Perches

[permalink] [raw]

Subject: [PATCH 04/11] dvb-net: Fix probable mask then right shift defects

2014-10-27 05:25:32

by Joe Perches

[permalink] [raw]

Subject: [PATCH 05/11] cx25840/cx18: Use standard ordering of mask and shift

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

This use has a mask then shift which is not the normal style.

Move the shift before the mask to match nearly all the other
uses in kernel.

Signed-off-by: Joe Perches <[email protected]>
---
drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
drivers/media/pci/cx18/cx18-av-core.c | 16 ++++++++--------
2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/cx25840/cx25840-core.c b/drivers/media/i2c/cx25840/cx25840-core.c
index e453a3f..0327032 100644
--- a/drivers/media/i2c/cx25840/cx25840-core.c
+++ b/drivers/media/i2c/cx25840/cx25840-core.c
@@ -879,7 +879,7 @@ void cx25840_std_setup(struct i2c_client *client)
/* Sets horizontal blanking delay and active lines */
cx25840_write(client, 0x470, hblank);
cx25840_write(client, 0x471,
- 0xff & (((hblank >> 8) & 0x3) | (hactive << 4)));
+ (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
cx25840_write(client, 0x472, hactive >> 4);

/* Sets burst gate delay */
@@ -888,13 +888,13 @@ void cx25840_std_setup(struct i2c_client *client)
/* Sets vertical blanking delay and active duration */
cx25840_write(client, 0x474, vblank);
cx25840_write(client, 0x475,
- 0xff & (((vblank >> 8) & 0x3) | (vactive << 4)));
+ (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
cx25840_write(client, 0x476, vactive >> 4);
cx25840_write(client, 0x477, vblank656);

/* Sets src decimation rate */
- cx25840_write(client, 0x478, 0xff & src_decimation);
- cx25840_write(client, 0x479, 0xff & (src_decimation >> 8));
+ cx25840_write(client, 0x478, src_decimation & 0xff);
+ cx25840_write(client, 0x479, (src_decimation >> 8) & 0xff);

/* Sets Luma and UV Low pass filters */
cx25840_write(client, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));
@@ -904,8 +904,8 @@ void cx25840_std_setup(struct i2c_client *client)

/* Sets SC Step*/
cx25840_write(client, 0x47c, sc);
- cx25840_write(client, 0x47d, 0xff & sc >> 8);
- cx25840_write(client, 0x47e, 0xff & sc >> 16);
+ cx25840_write(client, 0x47d, (sc >> 8) & 0xff);
+ cx25840_write(client, 0x47e, (sc >> 16) & 0xff);

/* Sets VBI parameters */
if (std & V4L2_STD_625_50) {
diff --git a/drivers/media/pci/cx18/cx18-av-core.c b/drivers/media/pci/cx18/cx18-av-core.c
index 2d3afe0..45be26c 100644
--- a/drivers/media/pci/cx18/cx18-av-core.c
+++ b/drivers/media/pci/cx18/cx18-av-core.c
@@ -490,8 +490,8 @@ void cx18_av_std_setup(struct cx18 *cx)

/* Sets horizontal blanking delay and active lines */
cx18_av_write(cx, 0x470, hblank);
- cx18_av_write(cx, 0x471, 0xff & (((hblank >> 8) & 0x3) |
- (hactive << 4)));
+ cx18_av_write(cx, 0x471,
+ (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
cx18_av_write(cx, 0x472, hactive >> 4);

/* Sets burst gate delay */
@@ -499,14 +499,14 @@ void cx18_av_std_setup(struct cx18 *cx)

/* Sets vertical blanking delay and active duration */
cx18_av_write(cx, 0x474, vblank);
- cx18_av_write(cx, 0x475, 0xff & (((vblank >> 8) & 0x3) |
- (vactive << 4)));
+ cx18_av_write(cx, 0x475,
+ (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
cx18_av_write(cx, 0x476, vactive >> 4);
cx18_av_write(cx, 0x477, vblank656);

/* Sets src decimation rate */
- cx18_av_write(cx, 0x478, 0xff & src_decimation);
- cx18_av_write(cx, 0x479, 0xff & (src_decimation >> 8));
+ cx18_av_write(cx, 0x478, src_decimation & 0xff);
+ cx18_av_write(cx, 0x479, (src_decimation >> 8) & 0xff);

/* Sets Luma and UV Low pass filters */
cx18_av_write(cx, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));
@@ -516,8 +516,8 @@ void cx18_av_std_setup(struct cx18 *cx)

/* Sets SC Step*/
cx18_av_write(cx, 0x47c, sc);
- cx18_av_write(cx, 0x47d, 0xff & sc >> 8);
- cx18_av_write(cx, 0x47e, 0xff & sc >> 16);
+ cx18_av_write(cx, 0x47d, (sc >> 8) & 0xff);
+ cx18_av_write(cx, 0x47e, (sc >> 16) & 0xff);

if (std & V4L2_STD_625_50) {
state->slicer_line_delay = 1;
--
2.1.2

2014-10-27 05:25:37

by Joe Perches

[permalink] [raw]

Subject: [PATCH 07/11] iwlwifi: dvm: Fix probable mask then right shift defect

2014-10-27 05:25:41

by Joe Perches

[permalink] [raw]

Subject: [PATCH 09/11] tty: ipwireless: Fix probable mask then right shift defects

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

Add parentheses around the masks.

Signed-off-by: Joe Perches <[email protected]>
---
drivers/tty/ipwireless/hardware.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/ipwireless/hardware.c b/drivers/tty/ipwireless/hardware.c
index 2c14842..5c77e1e 100644
--- a/drivers/tty/ipwireless/hardware.c
+++ b/drivers/tty/ipwireless/hardware.c
@@ -378,9 +378,9 @@ static void swap_packet_bitfield_to_le(unsigned char *data)
/*
* transform bits from aa.bbb.ccc to ccc.bbb.aa
*/
- ret |= tmp & 0xc0 >> 6;
- ret |= tmp & 0x38 >> 1;
- ret |= tmp & 0x07 << 5;
+ ret |= (tmp & 0xc0) >> 6;
+ ret |= (tmp & 0x38) >> 1;
+ ret |= (tmp & 0x07) << 5;
*data = ret & 0xff;
#endif
}
@@ -393,9 +393,9 @@ static void swap_packet_bitfield_from_le(unsigned char *data)
/*
* transform bits from ccc.bbb.aa to aa.bbb.ccc
*/
- ret |= tmp & 0xe0 >> 5;
- ret |= tmp & 0x1c << 1;
- ret |= tmp & 0x03 << 6;
+ ret |= (tmp & 0xe0) >> 5;
+ ret |= (tmp & 0x1c) << 1;
+ ret |= (tmp & 0x03) << 6;
*data = ret & 0xff;
#endif
}
--
2.1.2

2014-10-27 05:25:47

by Joe Perches

[permalink] [raw]

Subject: [PATCH 11/11] sound: ad1889: Fix probable mask then right shift defects

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Signed-off-by: Joe Perches <[email protected]>
---
sound/pci/ad1889.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/pci/ad1889.c b/sound/pci/ad1889.c
index 7bfdf9c..1610c38 100644
--- a/sound/pci/ad1889.c
+++ b/sound/pci/ad1889.c
@@ -681,7 +681,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe

/* WARQ is at offset 12 */
tmp = (reg & AD_DS_WSMC_WARQ) ?
- (((reg & AD_DS_WSMC_WARQ >> 12) & 0x01) ? 12 : 18) : 4;
+ ((((reg & AD_DS_WSMC_WARQ) >> 12) & 0x01) ? 12 : 18) : 4;
tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;

snd_iprintf(buffer, "Wave FIFO: %d %s words\n\n", tmp,
@@ -693,7 +693,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe

/* SYRQ is at offset 4 */
tmp = (reg & AD_DS_WSMC_SYRQ) ?
- (((reg & AD_DS_WSMC_SYRQ >> 4) & 0x01) ? 12 : 18) : 4;
+ ((((reg & AD_DS_WSMC_SYRQ) >> 4) & 0x01) ? 12 : 18) : 4;
tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;

snd_iprintf(buffer, "Synthesis FIFO: %d %s words\n\n", tmp,
@@ -709,7 +709,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe

/* ACRQ is at offset 4 */
tmp = (reg & AD_DS_RAMC_ACRQ) ?
- (((reg & AD_DS_RAMC_ACRQ >> 4) & 0x01) ? 12 : 18) : 4;
+ ((((reg & AD_DS_RAMC_ACRQ) >> 4) & 0x01) ? 12 : 18) : 4;
tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;

snd_iprintf(buffer, "ADC FIFO: %d %s words\n\n", tmp,
@@ -720,7 +720,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe

/* RERQ is at offset 12 */
tmp = (reg & AD_DS_RAMC_RERQ) ?
- (((reg & AD_DS_RAMC_RERQ >> 12) & 0x01) ? 12 : 18) : 4;
+ ((((reg & AD_DS_RAMC_RERQ) >> 12) & 0x01) ? 12 : 18) : 4;
tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;

snd_iprintf(buffer, "Resampler FIFO: %d %s words\n\n", tmp,
--
2.1.2

2014-10-27 05:26:03

by Joe Perches

[permalink] [raw]

Subject: [PATCH 10/11] hwa-hc: Fix probable mask then right shift defect

2014-10-27 05:26:38

by Joe Perches

[permalink] [raw]

Subject: [PATCH 08/11] ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect

2014-10-27 05:27:06

by Joe Perches

[permalink] [raw]

Subject: [PATCH 06/11] wm8350-core: Fix probable mask then right shift defect

2014-10-27 05:27:54

by Joe Perches

[permalink] [raw]

Subject: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

Here the shifts are unnecessary and a non-zero value can be used
as the test to set 1 or zero.

Signed-off-by: Joe Perches <[email protected]>
---
drivers/input/tablet/aiptek.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
index e7f966d..dee2bb9 100644
--- a/drivers/input/tablet/aiptek.c
+++ b/drivers/input/tablet/aiptek.c
@@ -489,9 +489,9 @@ static void aiptek_irq(struct urb *urb)
*/
jitterable = data[1] & 0x07;

- left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0;
- right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0;
- middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0;
+ left = data[1] & aiptek->curSetting.mouseButtonLeft ? 1 : 0;
+ right = data[1] & aiptek->curSetting.mouseButtonRight ? 1 : 0;
+ middle = data[1] & aiptek->curSetting.mouseButtonMiddle ? 1 : 0;

input_report_key(inputdev, BTN_LEFT, left);
input_report_key(inputdev, BTN_MIDDLE, middle);
--
2.1.2

2014-10-27 05:28:23

by Joe Perches

[permalink] [raw]

Subject: [PATCH 02/11] radeon: evergreen: Fix probable mask then right shift defects

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Signed-off-by: Joe Perches <[email protected]>
---
drivers/gpu/drm/radeon/evergreen.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index a31f1ca..a97a685 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
rdev->config.evergreen.tile_config |=
((gb_addr_config & 0x30000000) >> 28) << 12;

- num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
+ num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
+ >> NUM_SHADER_ENGINES) + 1;

if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
u32 efuse_straps_4;
--
2.1.2

2014-10-27 06:14:36

by Grumbach, Emmanuel

[permalink] [raw]

Subject: RE: [PATCH 07/11] iwlwifi: dvm: Fix probable mask then right shift defect

>
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---

Applied - thanks.

2014-10-27 07:41:24

by Takashi Iwai

[permalink] [raw]

Subject: Re: [PATCH 11/11] sound: ad1889: Fix probable mask then right shift defects

At Sun, 26 Oct 2014 22:25:07 -0700,
Joe Perches wrote:
>
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Signed-off-by: Joe Perches <[email protected]>

Thanks, applied.

Takashi

> ---
> sound/pci/ad1889.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sound/pci/ad1889.c b/sound/pci/ad1889.c
> index 7bfdf9c..1610c38 100644
> --- a/sound/pci/ad1889.c
> +++ b/sound/pci/ad1889.c
> @@ -681,7 +681,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
>
> /* WARQ is at offset 12 */
> tmp = (reg & AD_DS_WSMC_WARQ) ?
> - (((reg & AD_DS_WSMC_WARQ >> 12) & 0x01) ? 12 : 18) : 4;
> + ((((reg & AD_DS_WSMC_WARQ) >> 12) & 0x01) ? 12 : 18) : 4;
> tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;
>
> snd_iprintf(buffer, "Wave FIFO: %d %s words\n\n", tmp,
> @@ -693,7 +693,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
>
> /* SYRQ is at offset 4 */
> tmp = (reg & AD_DS_WSMC_SYRQ) ?
> - (((reg & AD_DS_WSMC_SYRQ >> 4) & 0x01) ? 12 : 18) : 4;
> + ((((reg & AD_DS_WSMC_SYRQ) >> 4) & 0x01) ? 12 : 18) : 4;
> tmp /= (reg & AD_DS_WSMC_WAST) ? 2 : 1;
>
> snd_iprintf(buffer, "Synthesis FIFO: %d %s words\n\n", tmp,
> @@ -709,7 +709,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
>
> /* ACRQ is at offset 4 */
> tmp = (reg & AD_DS_RAMC_ACRQ) ?
> - (((reg & AD_DS_RAMC_ACRQ >> 4) & 0x01) ? 12 : 18) : 4;
> + ((((reg & AD_DS_RAMC_ACRQ) >> 4) & 0x01) ? 12 : 18) : 4;
> tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;
>
> snd_iprintf(buffer, "ADC FIFO: %d %s words\n\n", tmp,
> @@ -720,7 +720,7 @@ snd_ad1889_proc_read(struct snd_info_entry *entry, struct snd_info_buffer *buffe
>
> /* RERQ is at offset 12 */
> tmp = (reg & AD_DS_RAMC_RERQ) ?
> - (((reg & AD_DS_RAMC_RERQ >> 12) & 0x01) ? 12 : 18) : 4;
> + ((((reg & AD_DS_RAMC_RERQ) >> 12) & 0x01) ? 12 : 18) : 4;
> tmp /= (reg & AD_DS_RAMC_ADST) ? 2 : 1;
>
> snd_iprintf(buffer, "Resampler FIFO: %d %s words\n\n", tmp,
> --
> 2.1.2
>

2014-10-27 09:20:29

by Michel Dänzer

[permalink] [raw]

Subject: Re: [PATCH 02/11] radeon: evergreen: Fix probable mask then right shift defects

On 27.10.2014 14:24, Joe Perches wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Use the already #defined values instead of hardcoding.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> drivers/gpu/drm/radeon/evergreen.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index a31f1ca..a97a685 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
> rdev->config.evergreen.tile_config |=
> ((gb_addr_config & 0x30000000) >> 28) << 12;
>
> - num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
> + num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
> + >> NUM_SHADER_ENGINES) + 1;
^^^^^^^^^^^^^^^^^^
I think this should be NUM_SHADER_ENGINES_SHIFT?

Looks good to me other than that.

--
Earthling Michel D?nzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer

2014-10-27 12:47:34

by Charles Keepax

[permalink] [raw]

Subject: Re: [PATCH 06/11] wm8350-core: Fix probable mask then right shift defect

On Sun, Oct 26, 2014 at 10:25:02PM -0700, Joe Perches wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---

Acked-by: Charles Keepax <[email protected]>

Thanks,
Charles

2014-10-27 13:18:37

by David Sterba

[permalink] [raw]

Subject: Re: [PATCH 09/11] tty: ipwireless: Fix probable mask then right shift defects

On Sun, Oct 26, 2014 at 10:25:05PM -0700, Joe Perches wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the masks.
>
> Signed-off-by: Joe Perches <[email protected]>

Acked-by: David Sterba <[email protected]>

Thanks.

2014-10-27 14:15:05

by Joe Perches

[permalink] [raw]

Subject: [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

Add parentheses around the mask.

Use the already #defined values instead of hardcoding.

Signed-off-by: Joe Perches <[email protected]>
---
> I think this should be NUM_SHADER_ENGINES_SHIFT?

(Joe can't type)

exactly right, thanks Michel

drivers/gpu/drm/radeon/evergreen.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index a31f1ca..a97a685 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
rdev->config.evergreen.tile_config |=
((gb_addr_config & 0x30000000) >> 28) << 12;

- num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
+ num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
+ >> NUM_SHADER_ENGINES_SHIFT) + 1;

if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
u32 efuse_straps_4;

2014-10-27 14:17:31

by Jiri Kosina

[permalink] [raw]

Subject: Re: [PATCH 09/11] tty: ipwireless: Fix probable mask then right shift defects

On Sun, 26 Oct 2014, Joe Perches wrote:

> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the masks.
>
> Signed-off-by: Joe Perches <[email protected]>

Reviewed-by: Jiri Kosina <[email protected]>

Greg, can you take this through your char/misc tree, please?

Thanks.

> ---
> drivers/tty/ipwireless/hardware.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/ipwireless/hardware.c b/drivers/tty/ipwireless/hardware.c
> index 2c14842..5c77e1e 100644
> --- a/drivers/tty/ipwireless/hardware.c
> +++ b/drivers/tty/ipwireless/hardware.c
> @@ -378,9 +378,9 @@ static void swap_packet_bitfield_to_le(unsigned char *data)
> /*
> * transform bits from aa.bbb.ccc to ccc.bbb.aa
> */
> - ret |= tmp & 0xc0 >> 6;
> - ret |= tmp & 0x38 >> 1;
> - ret |= tmp & 0x07 << 5;
> + ret |= (tmp & 0xc0) >> 6;
> + ret |= (tmp & 0x38) >> 1;
> + ret |= (tmp & 0x07) << 5;
> *data = ret & 0xff;
> #endif
> }
> @@ -393,9 +393,9 @@ static void swap_packet_bitfield_from_le(unsigned char *data)
> /*
> * transform bits from ccc.bbb.aa to aa.bbb.ccc
> */
> - ret |= tmp & 0xe0 >> 5;
> - ret |= tmp & 0x1c << 1;
> - ret |= tmp & 0x03 << 6;
> + ret |= (tmp & 0xe0) >> 5;
> + ret |= (tmp & 0x1c) << 1;
> + ret |= (tmp & 0x03) << 6;
> *data = ret & 0xff;
> #endif
> }
> --
> 2.1.2
>

--
Jiri Kosina
SUSE Labs

2014-10-27 14:44:17

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

Hi Joe,

On Sun, Oct 26, 2014 at 10:24:59PM -0700, Joe Perches wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.

Looking at the protocol description the current code is exactly right.
We want to "move" button bits first as in packet type 1 they are in a
different place than in other packets.

I'll take a patch that adds parenthesis around shifts to make clear it
is intended.

Thanks.

>
> Here the shifts are unnecessary and a non-zero value can be used
> as the test to set 1 or zero.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> drivers/input/tablet/aiptek.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
> index e7f966d..dee2bb9 100644
> --- a/drivers/input/tablet/aiptek.c
> +++ b/drivers/input/tablet/aiptek.c
> @@ -489,9 +489,9 @@ static void aiptek_irq(struct urb *urb)
> */
> jitterable = data[1] & 0x07;
>
> - left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0;
> - right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0;
> - middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0;
> + left = data[1] & aiptek->curSetting.mouseButtonLeft ? 1 : 0;
> + right = data[1] & aiptek->curSetting.mouseButtonRight ? 1 : 0;
> + middle = data[1] & aiptek->curSetting.mouseButtonMiddle ? 1 : 0;
>
> input_report_key(inputdev, BTN_LEFT, left);
> input_report_key(inputdev, BTN_MIDDLE, middle);
> --
> 2.1.2
>

--
Dmitry

2014-10-27 17:36:09

by Michael Büsch

[permalink] [raw]

Subject: Re: [PATCH 08/11] ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect

On Sun, 26 Oct 2014 22:25:04 -0700
Joe Perches <[email protected]> wrote:

> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Signed-off-by: Joe Perches <[email protected]>

Good catch.

Reviewed-by: Michael Büsch <[email protected]>

> ---
> drivers/ssb/driver_chipcommon_pmu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ssb/driver_chipcommon_pmu.c b/drivers/ssb/driver_chipcommon_pmu.c
> index 1173a09..bc71583 100644
> --- a/drivers/ssb/driver_chipcommon_pmu.c
> +++ b/drivers/ssb/driver_chipcommon_pmu.c
> @@ -621,8 +621,8 @@ static u32 ssb_pmu_get_alp_clock_clk0(struct ssb_chipcommon *cc)
> u32 crystalfreq;
> const struct pmu0_plltab_entry *e = NULL;
>
> - crystalfreq = chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
> - SSB_CHIPCO_PMU_CTL_XTALFREQ >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
> + crystalfreq = (chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
> + SSB_CHIPCO_PMU_CTL_XTALFREQ) >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
> e = pmu0_plltab_find_entry(crystalfreq);
> BUG_ON(!e);
> return e->freq * 1000;

--
Michael

Attachments:

(No filename) (819.00 B)
OpenPGP digital signature

2014-10-27 17:56:59

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

On Mon, 2014-10-27 at 07:44 -0700, Dmitry Torokhov wrote:
> Hi Joe,

Hello Dmitry.

> On Sun, Oct 26, 2014 at 10:24:59PM -0700, Joe Perches wrote:
> > Precedence of & and >> is not the same and is not left to right.
> > shift has higher precedence and should be done after the mask.
>
> Looking at the protocol description the current code is exactly right.
> We want to "move" button bits first as in packet type 1 they are in a
> different place than in other packets.
>
> I'll take a patch that adds parenthesis around shifts to make clear it
> is intended.

I think it's more sensible to do the shift first to a
temporary then direct comparisons.

Perhaps something like this cleanup that also uses
a temporary for aiptek->curSetting and
!!(foo & mask) instead of ((foo & mask) != 0) ? 1 : 0;
---
drivers/input/tablet/aiptek.c | 149 ++++++++++++++++++++----------------------
1 file changed, 72 insertions(+), 77 deletions(-)

diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
index e7f966d..9c46618 100644
--- a/drivers/input/tablet/aiptek.c
+++ b/drivers/input/tablet/aiptek.c
@@ -433,6 +433,7 @@ static const char *map_val_to_str(const struct aiptek_map *map, int val)
static void aiptek_irq(struct urb *urb)
{
struct aiptek *aiptek = urb->context;
+ struct aiptek_settings *settings = &aiptek->curSetting;
unsigned char *data = aiptek->data;
struct input_dev *inputdev = aiptek->inputdev;
struct usb_interface *intf = aiptek->intf;
@@ -472,26 +473,31 @@ static void aiptek_irq(struct urb *urb)
* tool generated the event.
*/
if (data[0] == 1) {
- if (aiptek->curSetting.coordinateMode ==
+ if (settings->coordinateMode ==
AIPTEK_COORDINATE_ABSOLUTE_MODE) {
aiptek->diagnostic =
AIPTEK_DIAGNOSTIC_SENDING_RELATIVE_IN_ABSOLUTE;
} else {
- x = (signed char) data[2];
- y = (signed char) data[3];
-
- /* jitterable keeps track of whether any button has been pressed.
- * We're also using it to remap the physical mouse button mask
- * to pseudo-settings. (We don't specifically care about it's
- * value after moving/transposing mouse button bitmasks, except
+ /*
+ * Shift buttons pressed to the curSettings location.
+ * jitterable keeps track of whether any button has
+ * been pressed. We're also using it to remap the
+ * physical mouse button mask to pseudo-settings.
+ * (We don't specifically care about it's value after
+ * moving/transposing mouse button bitmasks, except
* that a non-zero value indicates that one or more
* mouse button was pressed.)
*/
+ unsigned char buttons = data[1] << 2;
+
+ x = (signed char)data[2];
+ y = (signed char)data[3];
+
jitterable = data[1] & 0x07;

- left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0;
- right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0;
- middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0;
+ left = !!(buttons & settings->mouseButtonLeft);
+ right = !!(buttons & settings->mouseButtonRight);
+ middle = !!(buttons & settings->mouseButtonMiddle);

input_report_key(inputdev, BTN_LEFT, left);
input_report_key(inputdev, BTN_MIDDLE, middle);
@@ -505,10 +511,10 @@ static void aiptek_irq(struct urb *urb)
/* Wheel support is in the form of a single-event
* firing.
*/
- if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) {
+ if (settings->wheel != AIPTEK_WHEEL_DISABLE) {
input_report_rel(inputdev, REL_WHEEL,
- aiptek->curSetting.wheel);
- aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE;
+ settings->wheel);
+ settings->wheel = AIPTEK_WHEEL_DISABLE;
}
if (aiptek->lastMacro != -1) {
input_report_key(inputdev,
@@ -522,26 +528,26 @@ static void aiptek_irq(struct urb *urb)
* absolute coordinates.
*/
else if (data[0] == 2) {
- if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
+ if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE;
} else if (!AIPTEK_POINTER_ALLOW_STYLUS_MODE
- (aiptek->curSetting.pointerMode)) {
+ (settings->pointerMode)) {
aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED;
} else {
x = get_unaligned_le16(data + 1);
y = get_unaligned_le16(data + 3);
z = get_unaligned_le16(data + 6);

- dv = (data[5] & 0x01) != 0 ? 1 : 0;
- p = (data[5] & 0x02) != 0 ? 1 : 0;
- tip = (data[5] & 0x04) != 0 ? 1 : 0;
+ dv = !!(data[5] & 0x01);
+ p = !!(data[5] & 0x02);
+ tip = !!(data[5] & 0x04);

/* Use jitterable to re-arrange button masks
*/
jitterable = data[5] & 0x18;

- bs = (data[5] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0;
- pck = (data[5] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0;
+ bs = !!(data[5] & settings->stylusButtonLower);
+ pck = !!(data[5] & settings->stylusButtonUpper);

/* dv indicates 'data valid' (e.g., the tablet is in sync
* and has delivered a "correct" report) We will ignore
@@ -552,14 +558,14 @@ static void aiptek_irq(struct urb *urb)
* tool key, and set the new one.
*/
if (aiptek->previousToolMode !=
- aiptek->curSetting.toolMode) {
+ settings->toolMode) {
input_report_key(inputdev,
aiptek->previousToolMode, 0);
input_report_key(inputdev,
- aiptek->curSetting.toolMode,
+ settings->toolMode,
1);
aiptek->previousToolMode =
- aiptek->curSetting.toolMode;
+ settings->toolMode;
}

if (p != 0) {
@@ -571,27 +577,25 @@ static void aiptek_irq(struct urb *urb)
input_report_key(inputdev, BTN_STYLUS, bs);
input_report_key(inputdev, BTN_STYLUS2, pck);

- if (aiptek->curSetting.xTilt !=
- AIPTEK_TILT_DISABLE) {
+ if (settings->xTilt != AIPTEK_TILT_DISABLE) {
input_report_abs(inputdev,
ABS_TILT_X,
- aiptek->curSetting.xTilt);
+ settings->xTilt);
}
- if (aiptek->curSetting.yTilt != AIPTEK_TILT_DISABLE) {
+ if (settings->yTilt != AIPTEK_TILT_DISABLE) {
input_report_abs(inputdev,
ABS_TILT_Y,
- aiptek->curSetting.yTilt);
+ settings->yTilt);
}

/* Wheel support is in the form of a single-event
* firing.
*/
- if (aiptek->curSetting.wheel !=
- AIPTEK_WHEEL_DISABLE) {
+ if (settings->wheel != AIPTEK_WHEEL_DISABLE) {
input_report_abs(inputdev,
ABS_WHEEL,
- aiptek->curSetting.wheel);
- aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE;
+ settings->wheel);
+ settings->wheel = AIPTEK_WHEEL_DISABLE;
}
}
input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_STYLUS);
@@ -607,10 +611,10 @@ static void aiptek_irq(struct urb *urb)
/* Report 3's come from the mouse in absolute mode.
*/
else if (data[0] == 3) {
- if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
+ if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) {
aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE;
} else if (!AIPTEK_POINTER_ALLOW_MOUSE_MODE
- (aiptek->curSetting.pointerMode)) {
+ (settings->pointerMode)) {
aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED;
} else {
x = get_unaligned_le16(data + 1);
@@ -618,25 +622,25 @@ static void aiptek_irq(struct urb *urb)

jitterable = data[5] & 0x1c;

- dv = (data[5] & 0x01) != 0 ? 1 : 0;
- p = (data[5] & 0x02) != 0 ? 1 : 0;
- left = (data[5] & aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0;
- right = (data[5] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0;
- middle = (data[5] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0;
+ dv = !!(data[5] & 0x01);
+ p = !!(data[5] & 0x02);
+ left = !!(data[5] & settings->mouseButtonLeft);
+ right = !!(data[5] & settings->mouseButtonRight);
+ middle = !!(data[5] & settings->mouseButtonMiddle);

if (dv != 0) {
/* If the selected tool changed, reset the old
* tool key, and set the new one.
*/
if (aiptek->previousToolMode !=
- aiptek->curSetting.toolMode) {
+ settings->toolMode) {
input_report_key(inputdev,
aiptek->previousToolMode, 0);
input_report_key(inputdev,
- aiptek->curSetting.toolMode,
+ settings->toolMode,
1);
aiptek->previousToolMode =
- aiptek->curSetting.toolMode;
+ settings->toolMode;
}

if (p != 0) {
@@ -650,11 +654,11 @@ static void aiptek_irq(struct urb *urb)
/* Wheel support is in the form of a single-event
* firing.
*/
- if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) {
+ if (settings->wheel != AIPTEK_WHEEL_DISABLE) {
input_report_abs(inputdev,
ABS_WHEEL,
- aiptek->curSetting.wheel);
- aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE;
+ settings->wheel);
+ settings->wheel = AIPTEK_WHEEL_DISABLE;
}
}
input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_MOUSE);
@@ -672,11 +676,11 @@ static void aiptek_irq(struct urb *urb)
else if (data[0] == 4) {
jitterable = data[1] & 0x18;

- dv = (data[1] & 0x01) != 0 ? 1 : 0;
- p = (data[1] & 0x02) != 0 ? 1 : 0;
- tip = (data[1] & 0x04) != 0 ? 1 : 0;
- bs = (data[1] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0;
- pck = (data[1] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0;
+ dv = !!(data[1] & 0x01);
+ p = !!(data[1] & 0x02);
+ tip = !!(data[1] & 0x04);
+ bs = !!(data[1] & settings->stylusButtonLower);
+ pck = !!(data[1] & settings->stylusButtonUpper);

macro = dv && p && tip && !(data[3] & 1) ? (data[3] >> 1) : -1;
z = get_unaligned_le16(data + 4);
@@ -685,15 +689,12 @@ static void aiptek_irq(struct urb *urb)
/* If the selected tool changed, reset the old
* tool key, and set the new one.
*/
- if (aiptek->previousToolMode !=
- aiptek->curSetting.toolMode) {
+ if (aiptek->previousToolMode != settings->toolMode) {
input_report_key(inputdev,
aiptek->previousToolMode, 0);
input_report_key(inputdev,
- aiptek->curSetting.toolMode,
- 1);
- aiptek->previousToolMode =
- aiptek->curSetting.toolMode;
+ settings->toolMode, 1);
+ aiptek->previousToolMode = settings->toolMode;
}
}

@@ -715,24 +716,23 @@ static void aiptek_irq(struct urb *urb)
else if (data[0] == 5) {
jitterable = data[1] & 0x1c;

- dv = (data[1] & 0x01) != 0 ? 1 : 0;
- p = (data[1] & 0x02) != 0 ? 1 : 0;
- left = (data[1]& aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0;
- right = (data[1] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0;
- middle = (data[1] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0;
+ dv = !!(data[1] & 0x01);
+ p = !!(data[1] & 0x02);
+ left = !!(data[1]& settings->mouseButtonLeft);
+ right = !!(data[1] & settings->mouseButtonRight);
+ middle = !!(data[1] & settings->mouseButtonMiddle);
macro = dv && p && left && !(data[3] & 1) ? (data[3] >> 1) : 0;

if (dv) {
/* If the selected tool changed, reset the old
* tool key, and set the new one.
*/
- if (aiptek->previousToolMode !=
- aiptek->curSetting.toolMode) {
+ if (aiptek->previousToolMode != settings->toolMode) {
input_report_key(inputdev,
aiptek->previousToolMode, 0);
input_report_key(inputdev,
- aiptek->curSetting.toolMode, 1);
- aiptek->previousToolMode = aiptek->curSetting.toolMode;
+ settings->toolMode, 1);
+ aiptek->previousToolMode = settings->toolMode;
}
}

@@ -770,15 +770,10 @@ static void aiptek_irq(struct urb *urb)
/* If the selected tool changed, reset the old
tool key, and set the new one.
*/
- if (aiptek->previousToolMode !=
- aiptek->curSetting.toolMode) {
- input_report_key(inputdev,
- aiptek->previousToolMode, 0);
- input_report_key(inputdev,
- aiptek->curSetting.toolMode,
- 1);
- aiptek->previousToolMode =
- aiptek->curSetting.toolMode;
+ if (aiptek->previousToolMode != settings->toolMode) {
+ input_report_key(inputdev, aiptek->previousToolMode, 0);
+ input_report_key(inputdev, settings->toolMode, 1);
+ aiptek->previousToolMode = settings->toolMode;
}

input_report_key(inputdev, macroKeyEvents[macro], 1);
@@ -802,9 +797,9 @@ static void aiptek_irq(struct urb *urb)
*/

if (aiptek->previousJitterable != jitterable &&
- aiptek->curSetting.jitterDelay != 0 && aiptek->inDelay != 1) {
+ settings->jitterDelay != 0 && aiptek->inDelay != 1) {
aiptek->endDelay = jiffies +
- ((aiptek->curSetting.jitterDelay * HZ) / 1000);
+ ((settings->jitterDelay * HZ) / 1000);
aiptek->inDelay = 1;
}
aiptek->previousJitterable = jitterable;

2014-10-27 18:01:54

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

On Monday, October 27, 2014 10:56:54 AM Joe Perches wrote:
> On Mon, 2014-10-27 at 07:44 -0700, Dmitry Torokhov wrote:
> > Hi Joe,
>
> Hello Dmitry.
>
> > On Sun, Oct 26, 2014 at 10:24:59PM -0700, Joe Perches wrote:
> > > Precedence of & and >> is not the same and is not left to right.
> > > shift has higher precedence and should be done after the mask.
> >
> > Looking at the protocol description the current code is exactly right.
> > We want to "move" button bits first as in packet type 1 they are in a
> > different place than in other packets.
> >
> > I'll take a patch that adds parenthesis around shifts to make clear it
> > is intended.
>
> I think it's more sensible to do the shift first to a
> temporary then direct comparisons.
>
> Perhaps something like this cleanup that also uses
> a temporary for aiptek->curSetting and
> !!(foo & mask) instead of ((foo & mask) != 0) ? 1 : 0;

Unless you have the device I'd rather kept the changes (which are mostly
cosmetic in nature and do not fix any bugs) to a minimum.

Thanks.

--
Dmitry

2014-10-27 18:04:00

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects

On Mon, 2014-10-27 at 11:01 -0700, Dmitry Torokhov wrote:
> On Monday, October 27, 2014 10:56:54 AM Joe Perches wrote:
> > On Mon, 2014-10-27 at 07:44 -0700, Dmitry Torokhov wrote:
> > > On Sun, Oct 26, 2014 at 10:24:59PM -0700, Joe Perches wrote:
> > > > Precedence of & and >> is not the same and is not left to right.
> > > > shift has higher precedence and should be done after the mask.
> > >
> > > Looking at the protocol description the current code is exactly right.
> > > We want to "move" button bits first as in packet type 1 they are in a
> > > different place than in other packets.
> > >
> > > I'll take a patch that adds parenthesis around shifts to make clear it
> > > is intended.
> >
> > I think it's more sensible to do the shift first to a
> > temporary then direct comparisons.
[]
> Unless you have the device I'd rather kept the changes (which are mostly
> cosmetic in nature and do not fix any bugs) to a minimum.

I don't have the device.
I think you should do what you think appropriate.

cheers, Joe

2014-10-28 01:58:38

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 09/11] tty: ipwireless: Fix probable mask then right shift defects

On Mon, Oct 27, 2014 at 03:17:28PM +0100, Jiri Kosina wrote:
> On Sun, 26 Oct 2014, Joe Perches wrote:
>
> > Precedence of & and >> is not the same and is not left to right.
> > shift has higher precedence and should be done after the mask.
> >
> > Add parentheses around the masks.
> >
> > Signed-off-by: Joe Perches <[email protected]>
>
> Reviewed-by: Jiri Kosina <[email protected]>
>
> Greg, can you take this through your char/misc tree, please?

Ok, will do, give me a week or so to catch up on pending patches.

greg k-h

2014-10-28 07:29:00

by Michel Dänzer

[permalink] [raw]

Subject: Re: [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect

On 27.10.2014 23:14, Joe Perches wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Use the already #defined values instead of hardcoding.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
>> I think this should be NUM_SHADER_ENGINES_SHIFT?
>
> (Joe can't type)
>
> exactly right, thanks Michel
>
> drivers/gpu/drm/radeon/evergreen.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index a31f1ca..a97a685 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
> rdev->config.evergreen.tile_config |=
> ((gb_addr_config & 0x30000000) >> 28) << 12;
>
> - num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
> + num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
> + >> NUM_SHADER_ENGINES_SHIFT) + 1;
>
> if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
> u32 efuse_straps_4;

Reviewed-by: Michel Dänzer <[email protected]>

--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer

2014-10-28 14:06:22

by Alex Deucher

[permalink] [raw]

Subject: Re: [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect

On Mon, Oct 27, 2014 at 10:14 AM, Joe Perches <[email protected]> wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Use the already #defined values instead of hardcoding.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
>> I think this should be NUM_SHADER_ENGINES_SHIFT?
>
> (Joe can't type)
>
> exactly right, thanks Michel

Applied with a compile fix.

Thanks,

Alex

>
> drivers/gpu/drm/radeon/evergreen.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index a31f1ca..a97a685 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -3303,7 +3303,8 @@ static void evergreen_gpu_init(struct radeon_device *rdev)
> rdev->config.evergreen.tile_config |=
> ((gb_addr_config & 0x30000000) >> 28) << 12;
>
> - num_shader_engines = (gb_addr_config & NUM_SHADER_ENGINES(3) >> 12) + 1;
> + num_shader_engines = ((gb_addr_config & NUM_SHADER_ENGINES_MASK)
> + >> NUM_SHADER_ENGINES_SHIFT) + 1;
>
> if ((rdev->family >= CHIP_CEDAR) && (rdev->family <= CHIP_HEMLOCK)) {
> u32 efuse_straps_4;
>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

2014-10-29 03:03:44

by Michel Dänzer

[permalink] [raw]

Subject: Re: [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect

On 28.10.2014 23:06, Alex Deucher wrote:
> On Mon, Oct 27, 2014 at 10:14 AM, Joe Perches <[email protected]> wrote:
>> Precedence of & and >> is not the same and is not left to right.
>> shift has higher precedence and should be done after the mask.
>>
>> Add parentheses around the mask.
>>
>> Use the already #defined values instead of hardcoding.
>>
>> Signed-off-by: Joe Perches <[email protected]>
>> ---
>>> I think this should be NUM_SHADER_ENGINES_SHIFT?
>>
>> (Joe can't type)
>>
>> exactly right, thanks Michel
>
> Applied with a compile fix.

Joe, in the future please make sure your patches compile before
submitting them.

--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer

2014-11-03 18:02:27

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 06/11] wm8350-core: Fix probable mask then right shift defect

On Sun, 26 Oct 2014, Joe Perches wrote:

> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> drivers/mfd/wm8350-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Applied with Charles' Ack.

> diff --git a/drivers/mfd/wm8350-core.c b/drivers/mfd/wm8350-core.c
> index 4ab527f..f5124a8 100644
> --- a/drivers/mfd/wm8350-core.c
> +++ b/drivers/mfd/wm8350-core.c
> @@ -308,7 +308,7 @@ int wm8350_device_init(struct wm8350 *wm8350, int irq,
> goto err;
> }
>
> - mode = id2 & WM8350_CONF_STS_MASK >> 10;
> + mode = (id2 & WM8350_CONF_STS_MASK) >> 10;
> cust_id = id2 & WM8350_CUST_ID_MASK;
> chip_rev = (id2 & WM8350_CHIP_REV_MASK) >> 12;
> dev_info(wm8350->dev,

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2014-11-08 14:34:15

by Andy Walls

[permalink] [raw]

Subject: Re: [PATCH 05/11] cx25840/cx18: Use standard ordering of mask and shift

On Sun, 2014-10-26 at 22:25 -0700, Joe Perches wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> This use has a mask then shift which is not the normal style.
>
> Move the shift before the mask to match nearly all the other
> uses in kernel.
>
> Signed-off-by: Joe Perches <[email protected]>

The patch is technically correct.

Reviewed-by: Andy Walls <[email protected]>

> ---
> drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
> drivers/media/pci/cx18/cx18-av-core.c | 16 ++++++++--------
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/cx25840/cx25840-core.c b/drivers/media/i2c/cx25840/cx25840-core.c
> index e453a3f..0327032 100644
> --- a/drivers/media/i2c/cx25840/cx25840-core.c
> +++ b/drivers/media/i2c/cx25840/cx25840-core.c
> @@ -879,7 +879,7 @@ void cx25840_std_setup(struct i2c_client *client)
> /* Sets horizontal blanking delay and active lines */
> cx25840_write(client, 0x470, hblank);
> cx25840_write(client, 0x471,
> - 0xff & (((hblank >> 8) & 0x3) | (hactive << 4)));
> + (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
> cx25840_write(client, 0x472, hactive >> 4);
>
> /* Sets burst gate delay */
> @@ -888,13 +888,13 @@ void cx25840_std_setup(struct i2c_client *client)
> /* Sets vertical blanking delay and active duration */
> cx25840_write(client, 0x474, vblank);
> cx25840_write(client, 0x475,
> - 0xff & (((vblank >> 8) & 0x3) | (vactive << 4)));
> + (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
> cx25840_write(client, 0x476, vactive >> 4);
> cx25840_write(client, 0x477, vblank656);
>
> /* Sets src decimation rate */
> - cx25840_write(client, 0x478, 0xff & src_decimation);
> - cx25840_write(client, 0x479, 0xff & (src_decimation >> 8));
> + cx25840_write(client, 0x478, src_decimation & 0xff);
> + cx25840_write(client, 0x479, (src_decimation >> 8) & 0xff);
>
> /* Sets Luma and UV Low pass filters */
> cx25840_write(client, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));
> @@ -904,8 +904,8 @@ void cx25840_std_setup(struct i2c_client *client)
>
> /* Sets SC Step*/
> cx25840_write(client, 0x47c, sc);
> - cx25840_write(client, 0x47d, 0xff & sc >> 8);
> - cx25840_write(client, 0x47e, 0xff & sc >> 16);
> + cx25840_write(client, 0x47d, (sc >> 8) & 0xff);
> + cx25840_write(client, 0x47e, (sc >> 16) & 0xff);
>
> /* Sets VBI parameters */
> if (std & V4L2_STD_625_50) {
> diff --git a/drivers/media/pci/cx18/cx18-av-core.c b/drivers/media/pci/cx18/cx18-av-core.c
> index 2d3afe0..45be26c 100644
> --- a/drivers/media/pci/cx18/cx18-av-core.c
> +++ b/drivers/media/pci/cx18/cx18-av-core.c
> @@ -490,8 +490,8 @@ void cx18_av_std_setup(struct cx18 *cx)
>
> /* Sets horizontal blanking delay and active lines */
> cx18_av_write(cx, 0x470, hblank);
> - cx18_av_write(cx, 0x471, 0xff & (((hblank >> 8) & 0x3) |
> - (hactive << 4)));
> + cx18_av_write(cx, 0x471,
> + (((hblank >> 8) & 0x3) | (hactive << 4)) & 0xff);
> cx18_av_write(cx, 0x472, hactive >> 4);
>
> /* Sets burst gate delay */
> @@ -499,14 +499,14 @@ void cx18_av_std_setup(struct cx18 *cx)
>
> /* Sets vertical blanking delay and active duration */
> cx18_av_write(cx, 0x474, vblank);
> - cx18_av_write(cx, 0x475, 0xff & (((vblank >> 8) & 0x3) |
> - (vactive << 4)));
> + cx18_av_write(cx, 0x475,
> + (((vblank >> 8) & 0x3) | (vactive << 4)) & 0xff);
> cx18_av_write(cx, 0x476, vactive >> 4);
> cx18_av_write(cx, 0x477, vblank656);
>
> /* Sets src decimation rate */
> - cx18_av_write(cx, 0x478, 0xff & src_decimation);
> - cx18_av_write(cx, 0x479, 0xff & (src_decimation >> 8));
> + cx18_av_write(cx, 0x478, src_decimation & 0xff);
> + cx18_av_write(cx, 0x479, (src_decimation >> 8) & 0xff);
>
> /* Sets Luma and UV Low pass filters */
> cx18_av_write(cx, 0x47a, luma_lpf << 6 | ((uv_lpf << 4) & 0x30));
> @@ -516,8 +516,8 @@ void cx18_av_std_setup(struct cx18 *cx)
>
> /* Sets SC Step*/
> cx18_av_write(cx, 0x47c, sc);
> - cx18_av_write(cx, 0x47d, 0xff & sc >> 8);
> - cx18_av_write(cx, 0x47e, 0xff & sc >> 16);
> + cx18_av_write(cx, 0x47d, (sc >> 8) & 0xff);
> + cx18_av_write(cx, 0x47e, (sc >> 16) & 0xff);
>
> if (std & V4L2_STD_625_50) {
> state->slicer_line_delay = 1;

2014-11-10 19:34:26

by Vishal Verma

[permalink] [raw]

Subject: Re: [PATCH 01/11] block: nvme-scsi: Fix probable mask then right shift defects

On Sun, 2014-10-26 at 22:24 -0700, Joe Perches wrote:
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Signed-off-by: Joe Perches <[email protected]>

Acked-by: Vishal Verma <[email protected]>

> ---
> drivers/block/nvme-scsi.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/nvme-scsi.c b/drivers/block/nvme-scsi.c
> index a4cd6d6..529ee54 100644
> --- a/drivers/block/nvme-scsi.c
> +++ b/drivers/block/nvme-scsi.c
> @@ -1972,8 +1972,8 @@ static inline void nvme_trans_get_io_cdb10(u8 *cmd,
> {
> cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_10_CDB_FUA_OFFSET) &
> IO_CDB_FUA_MASK;
> - cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
> - IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
> + cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_10_CDB_WP_OFFSET) &
> + IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
> cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_10_CDB_LBA_OFFSET);
> cdb_info->xfer_len = GET_U16_FROM_CDB(cmd, IO_10_CDB_TX_LEN_OFFSET);
> }
> @@ -1983,8 +1983,8 @@ static inline void nvme_trans_get_io_cdb12(u8 *cmd,
> {
> cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_12_CDB_FUA_OFFSET) &
> IO_CDB_FUA_MASK;
> - cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
> - IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
> + cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_12_CDB_WP_OFFSET) &
> + IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
> cdb_info->lba = GET_U32_FROM_CDB(cmd, IO_12_CDB_LBA_OFFSET);
> cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_12_CDB_TX_LEN_OFFSET);
> }
> @@ -1994,8 +1994,8 @@ static inline void nvme_trans_get_io_cdb16(u8 *cmd,
> {
> cdb_info->fua = GET_U8_FROM_CDB(cmd, IO_16_CDB_FUA_OFFSET) &
> IO_CDB_FUA_MASK;
> - cdb_info->prot_info = GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
> - IO_CDB_WP_MASK >> IO_CDB_WP_SHIFT;
> + cdb_info->prot_info = (GET_U8_FROM_CDB(cmd, IO_16_CDB_WP_OFFSET) &
> + IO_CDB_WP_MASK) >> IO_CDB_WP_SHIFT;
> cdb_info->lba = GET_U64_FROM_CDB(cmd, IO_16_CDB_LBA_OFFSET);
> cdb_info->xfer_len = GET_U32_FROM_CDB(cmd, IO_16_CDB_TX_LEN_OFFSET);
> }

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?