* [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
@ 2016-06-08 4:07 ` Seung-Woo Kim
2016-06-09 12:38 ` Jaehoon Chung
2016-06-17 5:16 ` [PATCH v2] " Seung-Woo Kim
0 siblings, 2 replies; 11+ messages in thread
From: Seung-Woo Kim @ 2016-06-08 4:07 UTC (permalink / raw)
To: jh80.chung, ulf.hansson, linux-mmc, linux-kernel, sw0312.kim
This patch removes following UBSAN warnings in dw_mci_setup_bus().
The warnings are caused because of shift with more than 31 on 32
bit variable, so this patch fixes to shift only for less than 32.
UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
shift exponent 250 is too large for 32-bit type 'unsigned int'
Call trace:
[<ffffff90080908a8>] dump_backtrace+0x0/0x380
[<ffffff9008090c3c>] show_stack+0x14/0x20
[<ffffff90087457b8>] dump_stack+0xe0/0x120
[<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
[<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
[<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
[...]
UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
shift exponent 250 is too large for 32-bit type 'unsigned int'
Call trace:
[<ffffff90080908a8>] dump_backtrace+0x0/0x380
[<ffffff9008090c3c>] show_stack+0x14/0x20
[<ffffff90087457b8>] dump_stack+0xe0/0x120
[<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
[<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
[<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
[<ffffff9008d89ed4>] dw_mci_set_ios+0x184/0x798
[<ffffff9008d550ac>] mmc_power_up+0x11c/0x260
[<ffffff9008d56b78>] mmc_start_host+0x88/0x100
[<ffffff9008d583f4>] mmc_add_host+0x6c/0x128
[<ffffff9008d88f90>] dw_mci_probe+0x1088/0x1750
[<ffffff9008d8caa8>] dw_mci_pltfm_register+0x108/0x178
[<ffffff9008d8dae4>] dw_mci_exynos_probe+0x4c/0x88
[<ffffff9008a03838>] platform_drv_probe+0x78/0x180
[<ffffff90089fe9fc>] driver_probe_device+0x144/0x460
[<ffffff90089fee0c>] __driver_attach+0xf4/0x140
[<ffffff90089fad98>] bus_for_each_dev+0xf0/0x160
[<ffffff90089fdc9c>] driver_attach+0x34/0x58
[<ffffff90089fd3e0>] bus_add_driver+0x2c0/0x398
[<ffffff9008a00924>] driver_register+0xbc/0x1e0
[<ffffff9008a02a94>] __platform_driver_register+0x84/0xa8
[<ffffff9009dd5730>] dw_mci_exynos_pltfm_driver_init+0x18/0x20
[<ffffff9008082df8>] do_one_initcall+0xa0/0x2c8
[<ffffff9009d81780>] kernel_init_freeable+0x52c/0x5dc
[<ffffff9009519844>] kernel_init+0x1c/0xf8
[<ffffff9008086e10>] ret_from_fork+0x10/0x40
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 2cc6123..dff045e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
- if ((clock << div) != slot->__clk_old || force_clkinit)
+ if (((div < 32) ? (clock << div) : 0) != slot->__clk_old ||
+ force_clkinit)
dev_info(&slot->mmc->class_dev,
"Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
slot->id, host->bus_hz, clock,
@@ -1129,7 +1130,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
mci_send_cmd(slot, sdmmc_cmd_bits, 0);
/* keep the clock with reflecting clock dividor */
- slot->__clk_old = clock << div;
+ slot->__clk_old = (div < 32) ? (clock << div) : 0;
}
host->current_speed = clock;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-08 4:07 ` [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus() Seung-Woo Kim
@ 2016-06-09 12:38 ` Jaehoon Chung
2016-06-10 1:29 ` Seung-Woo Kim
2016-06-17 5:16 ` [PATCH v2] " Seung-Woo Kim
1 sibling, 1 reply; 11+ messages in thread
From: Jaehoon Chung @ 2016-06-09 12:38 UTC (permalink / raw)
To: Seung-Woo Kim, ulf.hansson, linux-mmc, linux-kernel
Hi Seung-Woo,
On 06/08/2016 01:07 PM, Seung-Woo Kim wrote:
> This patch removes following UBSAN warnings in dw_mci_setup_bus().
> The warnings are caused because of shift with more than 31 on 32
> bit variable, so this patch fixes to shift only for less than 32.
>
> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
> shift exponent 250 is too large for 32-bit type 'unsigned int'
> Call trace:
> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> [<ffffff9008090c3c>] show_stack+0x14/0x20
> [<ffffff90087457b8>] dump_stack+0xe0/0x120
> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> [<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
> [...]
>
> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
> shift exponent 250 is too large for 32-bit type 'unsigned int'
> Call trace:
> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> [<ffffff9008090c3c>] show_stack+0x14/0x20
> [<ffffff90087457b8>] dump_stack+0xe0/0x120
> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> [<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
The below message can be discarded.
> [<ffffff9008d89ed4>] dw_mci_set_ios+0x184/0x798
> [<ffffff9008d550ac>] mmc_power_up+0x11c/0x260
> [<ffffff9008d56b78>] mmc_start_host+0x88/0x100
> [<ffffff9008d583f4>] mmc_add_host+0x6c/0x128
> [<ffffff9008d88f90>] dw_mci_probe+0x1088/0x1750
> [<ffffff9008d8caa8>] dw_mci_pltfm_register+0x108/0x178
> [<ffffff9008d8dae4>] dw_mci_exynos_probe+0x4c/0x88
> [<ffffff9008a03838>] platform_drv_probe+0x78/0x180
> [<ffffff90089fe9fc>] driver_probe_device+0x144/0x460
> [<ffffff90089fee0c>] __driver_attach+0xf4/0x140
> [<ffffff90089fad98>] bus_for_each_dev+0xf0/0x160
> [<ffffff90089fdc9c>] driver_attach+0x34/0x58
> [<ffffff90089fd3e0>] bus_add_driver+0x2c0/0x398
> [<ffffff9008a00924>] driver_register+0xbc/0x1e0
> [<ffffff9008a02a94>] __platform_driver_register+0x84/0xa8
> [<ffffff9009dd5730>] dw_mci_exynos_pltfm_driver_init+0x18/0x20
> [<ffffff9008082df8>] do_one_initcall+0xa0/0x2c8
> [<ffffff9009d81780>] kernel_init_freeable+0x52c/0x5dc
> [<ffffff9009519844>] kernel_init+0x1c/0xf8
> [<ffffff9008086e10>] ret_from_fork+0x10/0x40
>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
> drivers/mmc/host/dw_mmc.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 2cc6123..dff045e 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
> div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
>
> - if ((clock << div) != slot->__clk_old || force_clkinit)
> + if (((div < 32) ? (clock << div) : 0) != slot->__clk_old ||
Well, we don't expect that clock is 0.
if clock is 0, it should be passed to " if (!clock)"..
During initializing card, clock is 400KHz or less..I understood what you want to fix.
But I taught this is not correct.
> + force_clkinit)
> dev_info(&slot->mmc->class_dev,
> "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> slot->id, host->bus_hz, clock,
> @@ -1129,7 +1130,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> mci_send_cmd(slot, sdmmc_cmd_bits, 0);
>
> /* keep the clock with reflecting clock dividor */
> - slot->__clk_old = clock << div;
> + slot->__clk_old = (div < 32) ? (clock << div) : 0;
Also, clock is not 0..
Best Regards,
Jaehoon Chung
> }
>
> host->current_speed = clock;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-09 12:38 ` Jaehoon Chung
@ 2016-06-10 1:29 ` Seung-Woo Kim
2016-06-17 1:30 ` Jaehoon Chung
0 siblings, 1 reply; 11+ messages in thread
From: Seung-Woo Kim @ 2016-06-10 1:29 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: ulf.hansson, linux-mmc, linux-kernel, Seung-Woo Kim
Hi Jaehoon,
On 2016년 06월 09일 21:38, Jaehoon Chung wrote:
> Hi Seung-Woo,
>
> On 06/08/2016 01:07 PM, Seung-Woo Kim wrote:
>> This patch removes following UBSAN warnings in dw_mci_setup_bus().
>> The warnings are caused because of shift with more than 31 on 32
>> bit variable, so this patch fixes to shift only for less than 32.
>>
>> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
>> shift exponent 250 is too large for 32-bit type 'unsigned int'
>> Call trace:
>> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
>> [<ffffff9008090c3c>] show_stack+0x14/0x20
>> [<ffffff90087457b8>] dump_stack+0xe0/0x120
>> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
>> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
>> [<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
>> [...]
>>
>> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
>> shift exponent 250 is too large for 32-bit type 'unsigned int'
>> Call trace:
>> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
>> [<ffffff9008090c3c>] show_stack+0x14/0x20
>> [<ffffff90087457b8>] dump_stack+0xe0/0x120
>> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
>> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
>> [<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
>
> The below message can be discarded.
You are right, like above message, below part can be replaced as [...].
>
>> [<ffffff9008d89ed4>] dw_mci_set_ios+0x184/0x798
>> [<ffffff9008d550ac>] mmc_power_up+0x11c/0x260
>> [<ffffff9008d56b78>] mmc_start_host+0x88/0x100
>> [<ffffff9008d583f4>] mmc_add_host+0x6c/0x128
>> [<ffffff9008d88f90>] dw_mci_probe+0x1088/0x1750
>> [<ffffff9008d8caa8>] dw_mci_pltfm_register+0x108/0x178
>> [<ffffff9008d8dae4>] dw_mci_exynos_probe+0x4c/0x88
>> [<ffffff9008a03838>] platform_drv_probe+0x78/0x180
>> [<ffffff90089fe9fc>] driver_probe_device+0x144/0x460
>> [<ffffff90089fee0c>] __driver_attach+0xf4/0x140
>> [<ffffff90089fad98>] bus_for_each_dev+0xf0/0x160
>> [<ffffff90089fdc9c>] driver_attach+0x34/0x58
>> [<ffffff90089fd3e0>] bus_add_driver+0x2c0/0x398
>> [<ffffff9008a00924>] driver_register+0xbc/0x1e0
>> [<ffffff9008a02a94>] __platform_driver_register+0x84/0xa8
>> [<ffffff9009dd5730>] dw_mci_exynos_pltfm_driver_init+0x18/0x20
>> [<ffffff9008082df8>] do_one_initcall+0xa0/0x2c8
>> [<ffffff9009d81780>] kernel_init_freeable+0x52c/0x5dc
>> [<ffffff9009519844>] kernel_init+0x1c/0xf8
>> [<ffffff9008086e10>] ret_from_fork+0x10/0x40
>>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>> ---
>> drivers/mmc/host/dw_mmc.c | 5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 2cc6123..dff045e 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>
>> div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
>>
>> - if ((clock << div) != slot->__clk_old || force_clkinit)
>> + if (((div < 32) ? (clock << div) : 0) != slot->__clk_old ||
>
> Well, we don't expect that clock is 0.
> if clock is 0, it should be passed to " if (!clock)"..
>
> During initializing card, clock is 400KHz or less..I understood what you want to fix.
> But I taught this is not correct.
It seems slot->__clk_old is not really clock but a kind of value to
store both clock and div in one variable. And at least, my compiler
calculates right shift with more than 32 as 0. I am not sure there is
other proper value for the case.
By the way, in my test environment, clock is calculated as like
following steps:
mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 400000Hz,
actual 400000HZ div = 250)
mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz,
actual 200000000HZ div = 0)
mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 52000000Hz,
actual 50000000HZ div = 2)
mmc_host mmc0: Bus speed (slot 0) = 400000000Hz (slot req 52000000Hz,
actual 50000000HZ div = 4)
mmc_host mmc0: Bus speed (slot 0) = 400000000Hz (slot req 200000000Hz,
actual 200000000HZ div = 1)
and only the first case is reported as the warning. If you let me know
any proper value, then I will fix with the value.
Thanks,
- Seung-Woo Kim
>
>
>> + force_clkinit)
>> dev_info(&slot->mmc->class_dev,
>> "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
>> slot->id, host->bus_hz, clock,
>> @@ -1129,7 +1130,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>> mci_send_cmd(slot, sdmmc_cmd_bits, 0);
>>
>> /* keep the clock with reflecting clock dividor */
>> - slot->__clk_old = clock << div;
>> + slot->__clk_old = (div < 32) ? (clock << div) : 0;
>
> Also, clock is not 0..
>
>
> Best Regards,
> Jaehoon Chung
>
>> }
>>
>> host->current_speed = clock;
>>
>
>
>
--
Seung-Woo Kim
Samsung Software R&D Center
--
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-10 1:29 ` Seung-Woo Kim
@ 2016-06-17 1:30 ` Jaehoon Chung
2016-06-17 4:07 ` Seung-Woo Kim
0 siblings, 1 reply; 11+ messages in thread
From: Jaehoon Chung @ 2016-06-17 1:30 UTC (permalink / raw)
To: sw0312.kim; +Cc: ulf.hansson, linux-mmc, linux-kernel
Hi Seung-Woo,
On 06/10/2016 10:29 AM, Seung-Woo Kim wrote:
> Hi Jaehoon,
>
> On 2016년 06월 09일 21:38, Jaehoon Chung wrote:
>> Hi Seung-Woo,
>>
>> On 06/08/2016 01:07 PM, Seung-Woo Kim wrote:
>>> This patch removes following UBSAN warnings in dw_mci_setup_bus().
>>> The warnings are caused because of shift with more than 31 on 32
>>> bit variable, so this patch fixes to shift only for less than 32.
>>>
>>> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
>>> shift exponent 250 is too large for 32-bit type 'unsigned int'
>>> Call trace:
>>> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
>>> [<ffffff9008090c3c>] show_stack+0x14/0x20
>>> [<ffffff90087457b8>] dump_stack+0xe0/0x120
>>> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
>>> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
>>> [<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
>>> [...]
>>>
>>> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
>>> shift exponent 250 is too large for 32-bit type 'unsigned int'
>>> Call trace:
>>> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
>>> [<ffffff9008090c3c>] show_stack+0x14/0x20
>>> [<ffffff90087457b8>] dump_stack+0xe0/0x120
>>> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
>>> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
>>> [<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
>>
>> The below message can be discarded.
>
> You are right, like above message, below part can be replaced as [...].
>
>>
>>> [<ffffff9008d89ed4>] dw_mci_set_ios+0x184/0x798
>>> [<ffffff9008d550ac>] mmc_power_up+0x11c/0x260
>>> [<ffffff9008d56b78>] mmc_start_host+0x88/0x100
>>> [<ffffff9008d583f4>] mmc_add_host+0x6c/0x128
>>> [<ffffff9008d88f90>] dw_mci_probe+0x1088/0x1750
>>> [<ffffff9008d8caa8>] dw_mci_pltfm_register+0x108/0x178
>>> [<ffffff9008d8dae4>] dw_mci_exynos_probe+0x4c/0x88
>>> [<ffffff9008a03838>] platform_drv_probe+0x78/0x180
>>> [<ffffff90089fe9fc>] driver_probe_device+0x144/0x460
>>> [<ffffff90089fee0c>] __driver_attach+0xf4/0x140
>>> [<ffffff90089fad98>] bus_for_each_dev+0xf0/0x160
>>> [<ffffff90089fdc9c>] driver_attach+0x34/0x58
>>> [<ffffff90089fd3e0>] bus_add_driver+0x2c0/0x398
>>> [<ffffff9008a00924>] driver_register+0xbc/0x1e0
>>> [<ffffff9008a02a94>] __platform_driver_register+0x84/0xa8
>>> [<ffffff9009dd5730>] dw_mci_exynos_pltfm_driver_init+0x18/0x20
>>> [<ffffff9008082df8>] do_one_initcall+0xa0/0x2c8
>>> [<ffffff9009d81780>] kernel_init_freeable+0x52c/0x5dc
>>> [<ffffff9009519844>] kernel_init+0x1c/0xf8
>>> [<ffffff9008086e10>] ret_from_fork+0x10/0x40
>>>
>>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>>> ---
>>> drivers/mmc/host/dw_mmc.c | 5 +++--
>>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 2cc6123..dff045e 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>>
>>> div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
>>>
>>> - if ((clock << div) != slot->__clk_old || force_clkinit)
>>> + if (((div < 32) ? (clock << div) : 0) != slot->__clk_old ||
>>
>> Well, we don't expect that clock is 0.
>> if clock is 0, it should be passed to " if (!clock)"..
>>
>> During initializing card, clock is 400KHz or less..I understood what you want to fix.
>> But I taught this is not correct.
>
> It seems slot->__clk_old is not really clock but a kind of value to
> store both clock and div in one variable. And at least, my compiler
> calculates right shift with more than 32 as 0. I am not sure there is
> other proper value for the case.
>
> By the way, in my test environment, clock is calculated as like
> following steps:
> mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 400000Hz,
> actual 400000HZ div = 250)
> mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz,
> actual 200000000HZ div = 0)
> mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 52000000Hz,
> actual 50000000HZ div = 2)
> mmc_host mmc0: Bus speed (slot 0) = 400000000Hz (slot req 52000000Hz,
> actual 50000000HZ div = 4)
> mmc_host mmc0: Bus speed (slot 0) = 400000000Hz (slot req 200000000Hz,
> actual 200000000HZ div = 1)
>
> and only the first case is reported as the warning. If you let me know
> any proper value, then I will fix with the value.
I think that "clock << div" and__old_clock are used for checking whether clock is changed.
So it doesn't need to rotate with div.
Could you check the below codes?
if ((clock != slot->clk_old) || force_clkinit)
...
slot->__clk_old = clock;
Best Regards,
Jaehoon Chung
>
> Thanks,
> - Seung-Woo Kim
>
>
>>
>>
>>> + force_clkinit)
>>> dev_info(&slot->mmc->class_dev,
>>> "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
>>> slot->id, host->bus_hz, clock,
>>> @@ -1129,7 +1130,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>> mci_send_cmd(slot, sdmmc_cmd_bits, 0);
>>>
>>> /* keep the clock with reflecting clock dividor */
>>> - slot->__clk_old = clock << div;
>>> + slot->__clk_old = (div < 32) ? (clock << div) : 0;
>>
>> Also, clock is not 0..
>>
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> }
>>>
>>> host->current_speed = clock;
>>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-17 1:30 ` Jaehoon Chung
@ 2016-06-17 4:07 ` Seung-Woo Kim
0 siblings, 0 replies; 11+ messages in thread
From: Seung-Woo Kim @ 2016-06-17 4:07 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: ulf.hansson, linux-mmc, linux-kernel, Seung-Woo Kim
Hello Jaehoon,
On 2016년 06월 17일 10:30, Jaehoon Chung wrote:
> Hi Seung-Woo,
>
> On 06/10/2016 10:29 AM, Seung-Woo Kim wrote:
>> Hi Jaehoon,
>>
>> On 2016년 06월 09일 21:38, Jaehoon Chung wrote:
>>> Hi Seung-Woo,
>>>
>>> On 06/08/2016 01:07 PM, Seung-Woo Kim wrote:
>>>> This patch removes following UBSAN warnings in dw_mci_setup_bus().
>>>> The warnings are caused because of shift with more than 31 on 32
>>>> bit variable, so this patch fixes to shift only for less than 32.
>>>>
<snip.>
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index 2cc6123..dff045e 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>>>
>>>> div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
>>>>
>>>> - if ((clock << div) != slot->__clk_old || force_clkinit)
>>>> + if (((div < 32) ? (clock << div) : 0) != slot->__clk_old ||
>>>
>>> Well, we don't expect that clock is 0.
>>> if clock is 0, it should be passed to " if (!clock)"..
>>>
>>> During initializing card, clock is 400KHz or less..I understood what you want to fix.
>>> But I taught this is not correct.
>>
>> It seems slot->__clk_old is not really clock but a kind of value to
>> store both clock and div in one variable. And at least, my compiler
>> calculates right shift with more than 32 as 0. I am not sure there is
>> other proper value for the case.
>>
>> By the way, in my test environment, clock is calculated as like
>> following steps:
>> mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 400000Hz,
>> actual 400000HZ div = 250)
>> mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz,
>> actual 200000000HZ div = 0)
>> mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 52000000Hz,
>> actual 50000000HZ div = 2)
>> mmc_host mmc0: Bus speed (slot 0) = 400000000Hz (slot req 52000000Hz,
>> actual 50000000HZ div = 4)
>> mmc_host mmc0: Bus speed (slot 0) = 400000000Hz (slot req 200000000Hz,
>> actual 200000000HZ div = 1)
>>
>> and only the first case is reported as the warning. If you let me know
>> any proper value, then I will fix with the value.
>
> I think that "clock << div" and__old_clock are used for checking whether clock is changed.
> So it doesn't need to rotate with div.
>
> Could you check the below codes?
>
> if ((clock != slot->clk_old) || force_clkinit)
> ...
>
> slot->__clk_old = clock;
>From the comment about alignment of slot->__clk_old, it says that "keep
the clock with reflecting clock dividor". So to me, it is better also
adding slot->__dlv with your suggestion. I will send patch like it, soon.
Regards,
- Seung-Woo Kim
>
> Best Regards,
> Jaehoon Chung
>
>>
>> Thanks,
>> - Seung-Woo Kim
>>
>>
>>>
>>>
>>>> + force_clkinit)
>>>> dev_info(&slot->mmc->class_dev,
>>>> "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
>>>> slot->id, host->bus_hz, clock,
>>>> @@ -1129,7 +1130,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>>> mci_send_cmd(slot, sdmmc_cmd_bits, 0);
>>>>
>>>> /* keep the clock with reflecting clock dividor */
>>>> - slot->__clk_old = clock << div;
>>>> + slot->__clk_old = (div < 32) ? (clock << div) : 0;
<snip.>
--
Seung-Woo Kim
Samsung Software R&D Center
--
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-08 4:07 ` [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus() Seung-Woo Kim
2016-06-09 12:38 ` Jaehoon Chung
@ 2016-06-17 5:16 ` Seung-Woo Kim
2016-06-20 2:34 ` Jaehoon Chung
2016-06-20 4:09 ` [PATCH v3] " Seung-Woo Kim
1 sibling, 2 replies; 11+ messages in thread
From: Seung-Woo Kim @ 2016-06-17 5:16 UTC (permalink / raw)
To: jh80.chung, ulf.hansson, linux-mmc, linux-kernel, sw0312.kim
This patch removes following UBSAN warnings in dw_mci_setup_bus().
UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
shift exponent 250 is too large for 32-bit type 'unsigned int'
Call trace:
[<ffffff90080908a8>] dump_backtrace+0x0/0x380
[<ffffff9008090c3c>] show_stack+0x14/0x20
[<ffffff90087457b8>] dump_stack+0xe0/0x120
[<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
[<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
[<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
[...]
UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
shift exponent 250 is too large for 32-bit type 'unsigned int'
Call trace:
[<ffffff90080908a8>] dump_backtrace+0x0/0x380
[<ffffff9008090c3c>] show_stack+0x14/0x20
[<ffffff90087457b8>] dump_stack+0xe0/0x120
[<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
[<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
[<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
[...]
The warnings are caused because of shift with more than 31 on 32
bit variable, so this patch fixes to keep both clock and divider
instead of shift.
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 8 +++++---
drivers/mmc/host/dw_mmc.h | 8 +++++---
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 2cc6123..d05c8cc 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
- if ((clock << div) != slot->__clk_old || force_clkinit)
+ if (clock != slot->__clk_old || div != slot->__div_old ||
+ force_clkinit)
dev_info(&slot->mmc->class_dev,
"Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
slot->id, host->bus_hz, clock,
@@ -1128,8 +1129,9 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
/* inform CIU */
mci_send_cmd(slot, sdmmc_cmd_bits, 0);
- /* keep the clock with reflecting clock dividor */
- slot->__clk_old = clock << div;
+ /* keep the clock and clock divider */
+ slot->__clk_old = clock;
+ slot->__div_old = div;
}
host->current_speed = clock;
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 1e8d838..fdfc3f5 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -245,9 +245,10 @@ extern int dw_mci_resume(struct dw_mci *host);
* @queue_node: List node for placing this node in the @queue list of
* &struct dw_mci.
* @clock: Clock rate configured by set_ios(). Protected by host->lock.
- * @__clk_old: The last updated clock with reflecting clock divider.
- * Keeping track of this helps us to avoid spamming the console
- * with CONFIG_MMC_CLKGATE.
+ * @__clk_old: The last updated clock.
+ * @__div_old: The last updated clock divider.
+ * Keeping track of clock and clock divider helps us to avoid spamming
+ * the console with CONFIG_MMC_CLKGATE.
* @flags: Random state bits associated with the slot.
* @id: Number of this slot.
* @sdio_id: Number of this slot in the SDIO interrupt registers.
@@ -263,6 +264,7 @@ struct dw_mci_slot {
unsigned int clock;
unsigned int __clk_old;
+ unsigned int __div_old;
unsigned long flags;
#define DW_MMC_CARD_PRESENT 0
--
1.7.4.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-17 5:16 ` [PATCH v2] " Seung-Woo Kim
@ 2016-06-20 2:34 ` Jaehoon Chung
2016-06-20 3:30 ` Seung-Woo Kim
2016-06-20 4:09 ` [PATCH v3] " Seung-Woo Kim
1 sibling, 1 reply; 11+ messages in thread
From: Jaehoon Chung @ 2016-06-20 2:34 UTC (permalink / raw)
To: Seung-Woo Kim, ulf.hansson, linux-mmc, linux-kernel
Hi SeungWoo,
On 06/17/2016 02:16 PM, Seung-Woo Kim wrote:
> This patch removes following UBSAN warnings in dw_mci_setup_bus().
>
> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
> shift exponent 250 is too large for 32-bit type 'unsigned int'
> Call trace:
> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> [<ffffff9008090c3c>] show_stack+0x14/0x20
> [<ffffff90087457b8>] dump_stack+0xe0/0x120
> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> [<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
> [...]
>
> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
> shift exponent 250 is too large for 32-bit type 'unsigned int'
> Call trace:
> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> [<ffffff9008090c3c>] show_stack+0x14/0x20
> [<ffffff90087457b8>] dump_stack+0xe0/0x120
> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> [<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
> [...]
>
> The warnings are caused because of shift with more than 31 on 32
> bit variable, so this patch fixes to keep both clock and divider
> instead of shift.
>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
> drivers/mmc/host/dw_mmc.c | 8 +++++---
> drivers/mmc/host/dw_mmc.h | 8 +++++---
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 2cc6123..d05c8cc 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
> div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
>
> - if ((clock << div) != slot->__clk_old || force_clkinit)
> + if (clock != slot->__clk_old || div != slot->__div_old ||
> + force_clkinit)
I have realized why we put this condition..after checking your below codes.
This was patch to avoid the spamming for CONFIG_MMC_CLKGATE..
but CONFIG_MMC_CLKAGET didn't use anymore.. I think we can remove this condition.
How about?
Best Regards,
Jaehoon Chung
> dev_info(&slot->mmc->class_dev,
> "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> slot->id, host->bus_hz, clock,
> @@ -1128,8 +1129,9 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> /* inform CIU */
> mci_send_cmd(slot, sdmmc_cmd_bits, 0);
>
> - /* keep the clock with reflecting clock dividor */
> - slot->__clk_old = clock << div;
> + /* keep the clock and clock divider */
> + slot->__clk_old = clock;
> + slot->__div_old = div;
> }
>
> host->current_speed = clock;
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 1e8d838..fdfc3f5 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -245,9 +245,10 @@ extern int dw_mci_resume(struct dw_mci *host);
> * @queue_node: List node for placing this node in the @queue list of
> * &struct dw_mci.
> * @clock: Clock rate configured by set_ios(). Protected by host->lock.
> - * @__clk_old: The last updated clock with reflecting clock divider.
> - * Keeping track of this helps us to avoid spamming the console
> - * with CONFIG_MMC_CLKGATE.
> + * @__clk_old: The last updated clock.
> + * @__div_old: The last updated clock divider.
> + * Keeping track of clock and clock divider helps us to avoid spamming
> + * the console with CONFIG_MMC_CLKGATE.
> * @flags: Random state bits associated with the slot.
> * @id: Number of this slot.
> * @sdio_id: Number of this slot in the SDIO interrupt registers.
> @@ -263,6 +264,7 @@ struct dw_mci_slot {
>
> unsigned int clock;
> unsigned int __clk_old;
> + unsigned int __div_old;
>
> unsigned long flags;
> #define DW_MMC_CARD_PRESENT 0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-20 2:34 ` Jaehoon Chung
@ 2016-06-20 3:30 ` Seung-Woo Kim
2016-06-20 3:45 ` Seung-Woo Kim
0 siblings, 1 reply; 11+ messages in thread
From: Seung-Woo Kim @ 2016-06-20 3:30 UTC (permalink / raw)
To: 'Jaehoon Chung', ulf.hansson, linux-mmc, linux-kernel; +Cc: sw0312.kim
Hello Jaehoon,
> -----Original Message-----
> From: Jaehoon Chung [mailto:jh80.chung@samsung.com]
> Sent: Monday, June 20, 2016 11:34 AM
> To: Seung-Woo Kim; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
>
> Hi SeungWoo,
>
> On 06/17/2016 02:16 PM, Seung-Woo Kim wrote:
> > This patch removes following UBSAN warnings in dw_mci_setup_bus().
> >
> > UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
> > shift exponent 250 is too large for 32-bit type 'unsigned int'
> > Call trace:
> > [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> > [<ffffff9008090c3c>] show_stack+0x14/0x20
> > [<ffffff90087457b8>] dump_stack+0xe0/0x120
> > [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> > [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> > [<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
> > [...]
> >
> > UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
> > shift exponent 250 is too large for 32-bit type 'unsigned int'
> > Call trace:
> > [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> > [<ffffff9008090c3c>] show_stack+0x14/0x20
> > [<ffffff90087457b8>] dump_stack+0xe0/0x120
> > [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> > [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> > [<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
> > [...]
> >
> > The warnings are caused because of shift with more than 31 on 32
> > bit variable, so this patch fixes to keep both clock and divider
> > instead of shift.
> >
> > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> > ---
> > drivers/mmc/host/dw_mmc.c | 8 +++++---
> > drivers/mmc/host/dw_mmc.h | 8 +++++---
> > 2 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index 2cc6123..d05c8cc 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> >
> > div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
> >
> > - if ((clock << div) != slot->__clk_old || force_clkinit)
> > + if (clock != slot->__clk_old || div != slot->__div_old ||
> > + force_clkinit)
>
> I have realized why we put this condition..after checking your below codes.
> This was patch to avoid the spamming for CONFIG_MMC_CLKGATE..
> but CONFIG_MMC_CLKAGET didn't use anymore.. I think we can remove this condition.
>
> How about?
There is no CONFIG_MMC_CLKAGET, but message is still printed. So without
the condition check, there will be too much log. If you will just remove
below print, then I agree about removing the condition and __clk_old, instead
of my patch.
Thanks,
- Seung-Woo Kim
>
> Best Regards,
> Jaehoon Chung
>
> > dev_info(&slot->mmc->class_dev,
> > "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> > slot->id, host->bus_hz, clock,
> > @@ -1128,8 +1129,9 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> > /* inform CIU */
> > mci_send_cmd(slot, sdmmc_cmd_bits, 0);
> >
> > - /* keep the clock with reflecting clock dividor */
> > - slot->__clk_old = clock << div;
> > + /* keep the clock and clock divider */
> > + slot->__clk_old = clock;
> > + slot->__div_old = div;
> > }
> >
> > host->current_speed = clock;
> > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> > index 1e8d838..fdfc3f5 100644
> > --- a/drivers/mmc/host/dw_mmc.h
> > +++ b/drivers/mmc/host/dw_mmc.h
> > @@ -245,9 +245,10 @@ extern int dw_mci_resume(struct dw_mci *host);
> > * @queue_node: List node for placing this node in the @queue list of
> > * &struct dw_mci.
> > * @clock: Clock rate configured by set_ios(). Protected by host->lock.
> > - * @__clk_old: The last updated clock with reflecting clock divider.
> > - * Keeping track of this helps us to avoid spamming the console
> > - * with CONFIG_MMC_CLKGATE.
> > + * @__clk_old: The last updated clock.
> > + * @__div_old: The last updated clock divider.
> > + * Keeping track of clock and clock divider helps us to avoid spamming
> > + * the console with CONFIG_MMC_CLKGATE.
> > * @flags: Random state bits associated with the slot.
> > * @id: Number of this slot.
> > * @sdio_id: Number of this slot in the SDIO interrupt registers.
> > @@ -263,6 +264,7 @@ struct dw_mci_slot {
> >
> > unsigned int clock;
> > unsigned int __clk_old;
> > + unsigned int __div_old;
> >
> > unsigned long flags;
> > #define DW_MMC_CARD_PRESENT 0
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-20 3:30 ` Seung-Woo Kim
@ 2016-06-20 3:45 ` Seung-Woo Kim
0 siblings, 0 replies; 11+ messages in thread
From: Seung-Woo Kim @ 2016-06-20 3:45 UTC (permalink / raw)
To: 'Jaehoon Chung', ulf.hansson, linux-mmc, linux-kernel; +Cc: sw0312.kim
Hi,
> -----Original Message-----
> From: Seung-Woo Kim [mailto:sw0312.kim@samsung.com]
> Sent: Monday, June 20, 2016 12:30 PM
> To: 'Jaehoon Chung'; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: sw0312.kim@samsung.com
> Subject: RE: [PATCH v2] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
>
> Hello Jaehoon,
>
> > -----Original Message-----
> > From: Jaehoon Chung [mailto:jh80.chung@samsung.com]
> > Sent: Monday, June 20, 2016 11:34 AM
> > To: Seung-Woo Kim; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
> >
> > Hi SeungWoo,
> >
> > On 06/17/2016 02:16 PM, Seung-Woo Kim wrote:
> > > This patch removes following UBSAN warnings in dw_mci_setup_bus().
> > >
> > > UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
> > > shift exponent 250 is too large for 32-bit type 'unsigned int'
> > > Call trace:
> > > [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> > > [<ffffff9008090c3c>] show_stack+0x14/0x20
> > > [<ffffff90087457b8>] dump_stack+0xe0/0x120
> > > [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> > > [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> > > [<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
> > > [...]
> > >
> > > UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
> > > shift exponent 250 is too large for 32-bit type 'unsigned int'
> > > Call trace:
> > > [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> > > [<ffffff9008090c3c>] show_stack+0x14/0x20
> > > [<ffffff90087457b8>] dump_stack+0xe0/0x120
> > > [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> > > [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> > > [<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
> > > [...]
> > >
> > > The warnings are caused because of shift with more than 31 on 32
> > > bit variable, so this patch fixes to keep both clock and divider
> > > instead of shift.
> > >
> > > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> > > ---
> > > drivers/mmc/host/dw_mmc.c | 8 +++++---
> > > drivers/mmc/host/dw_mmc.h | 8 +++++---
> > > 2 files changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > index 2cc6123..d05c8cc 100644
> > > --- a/drivers/mmc/host/dw_mmc.c
> > > +++ b/drivers/mmc/host/dw_mmc.c
> > > @@ -1099,7 +1099,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> > >
> > > div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
> > >
> > > - if ((clock << div) != slot->__clk_old || force_clkinit)
> > > + if (clock != slot->__clk_old || div != slot->__div_old ||
> > > + force_clkinit)
> >
> > I have realized why we put this condition..after checking your below codes.
> > This was patch to avoid the spamming for CONFIG_MMC_CLKGATE..
> > but CONFIG_MMC_CLKAGET didn't use anymore.. I think we can remove this condition.
> >
> > How about?
>
> There is no CONFIG_MMC_CLKAGET, but message is still printed. So without
> the condition check, there will be too much log. If you will just remove
> below print, then I agree about removing the condition and __clk_old, instead
> of my patch.
I tested again after removing the condition check, it prints same log and there
is not too much log. Let's just remove the condition and __clk_old.
Regards,
- Seung-Woo Kim
>
> Thanks,
> - Seung-Woo Kim
>
> >
> > Best Regards,
> > Jaehoon Chung
> >
> > > dev_info(&slot->mmc->class_dev,
> > > "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> > > slot->id, host->bus_hz, clock,
> > > @@ -1128,8 +1129,9 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> > > /* inform CIU */
> > > mci_send_cmd(slot, sdmmc_cmd_bits, 0);
> > >
> > > - /* keep the clock with reflecting clock dividor */
> > > - slot->__clk_old = clock << div;
> > > + /* keep the clock and clock divider */
> > > + slot->__clk_old = clock;
> > > + slot->__div_old = div;
> > > }
> > >
> > > host->current_speed = clock;
> > > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> > > index 1e8d838..fdfc3f5 100644
> > > --- a/drivers/mmc/host/dw_mmc.h
> > > +++ b/drivers/mmc/host/dw_mmc.h
> > > @@ -245,9 +245,10 @@ extern int dw_mci_resume(struct dw_mci *host);
> > > * @queue_node: List node for placing this node in the @queue list of
> > > * &struct dw_mci.
> > > * @clock: Clock rate configured by set_ios(). Protected by host->lock.
> > > - * @__clk_old: The last updated clock with reflecting clock divider.
> > > - * Keeping track of this helps us to avoid spamming the console
> > > - * with CONFIG_MMC_CLKGATE.
> > > + * @__clk_old: The last updated clock.
> > > + * @__div_old: The last updated clock divider.
> > > + * Keeping track of clock and clock divider helps us to avoid spamming
> > > + * the console with CONFIG_MMC_CLKGATE.
> > > * @flags: Random state bits associated with the slot.
> > > * @id: Number of this slot.
> > > * @sdio_id: Number of this slot in the SDIO interrupt registers.
> > > @@ -263,6 +264,7 @@ struct dw_mci_slot {
> > >
> > > unsigned int clock;
> > > unsigned int __clk_old;
> > > + unsigned int __div_old;
> > >
> > > unsigned long flags;
> > > #define DW_MMC_CARD_PRESENT 0
> > >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-17 5:16 ` [PATCH v2] " Seung-Woo Kim
2016-06-20 2:34 ` Jaehoon Chung
@ 2016-06-20 4:09 ` Seung-Woo Kim
2016-06-22 1:27 ` Jaehoon Chung
1 sibling, 1 reply; 11+ messages in thread
From: Seung-Woo Kim @ 2016-06-20 4:09 UTC (permalink / raw)
To: jh80.chung, ulf.hansson, linux-mmc, linux-kernel, sw0312.kim
This patch removes following UBSAN warnings in dw_mci_setup_bus().
UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
shift exponent 250 is too large for 32-bit type 'unsigned int'
Call trace:
[<ffffff90080908a8>] dump_backtrace+0x0/0x380
[<ffffff9008090c3c>] show_stack+0x14/0x20
[<ffffff90087457b8>] dump_stack+0xe0/0x120
[<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
[<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
[<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
[...]
UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
shift exponent 250 is too large for 32-bit type 'unsigned int'
Call trace:
[<ffffff90080908a8>] dump_backtrace+0x0/0x380
[<ffffff9008090c3c>] show_stack+0x14/0x20
[<ffffff90087457b8>] dump_stack+0xe0/0x120
[<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
[<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
[<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
[...]
The warnings are caused because of bit shift which is used to
filter spamming message for CONFIG_MMC_CLKGATE, but the config is
already removed. So this patch just removes the shift.
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 14 +++++---------
drivers/mmc/host/dw_mmc.h | 4 ----
2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 2cc6123..bada11e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1099,12 +1099,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
- if ((clock << div) != slot->__clk_old || force_clkinit)
- dev_info(&slot->mmc->class_dev,
- "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
- slot->id, host->bus_hz, clock,
- div ? ((host->bus_hz / div) >> 1) :
- host->bus_hz, div);
+ dev_info(&slot->mmc->class_dev,
+ "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
+ slot->id, host->bus_hz, clock,
+ div ? ((host->bus_hz / div) >> 1) :
+ host->bus_hz, div);
/* disable clock */
mci_writel(host, CLKENA, 0);
@@ -1127,9 +1126,6 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
/* inform CIU */
mci_send_cmd(slot, sdmmc_cmd_bits, 0);
-
- /* keep the clock with reflecting clock dividor */
- slot->__clk_old = clock << div;
}
host->current_speed = clock;
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 1e8d838..5961037 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -245,9 +245,6 @@ extern int dw_mci_resume(struct dw_mci *host);
* @queue_node: List node for placing this node in the @queue list of
* &struct dw_mci.
* @clock: Clock rate configured by set_ios(). Protected by host->lock.
- * @__clk_old: The last updated clock with reflecting clock divider.
- * Keeping track of this helps us to avoid spamming the console
- * with CONFIG_MMC_CLKGATE.
* @flags: Random state bits associated with the slot.
* @id: Number of this slot.
* @sdio_id: Number of this slot in the SDIO interrupt registers.
@@ -262,7 +259,6 @@ struct dw_mci_slot {
struct list_head queue_node;
unsigned int clock;
- unsigned int __clk_old;
unsigned long flags;
#define DW_MMC_CARD_PRESENT 0
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()
2016-06-20 4:09 ` [PATCH v3] " Seung-Woo Kim
@ 2016-06-22 1:27 ` Jaehoon Chung
0 siblings, 0 replies; 11+ messages in thread
From: Jaehoon Chung @ 2016-06-22 1:27 UTC (permalink / raw)
To: Seung-Woo Kim, ulf.hansson, linux-mmc, linux-kernel
Hi Seung-Woo,
On 06/20/2016 01:09 PM, Seung-Woo Kim wrote:
> This patch removes following UBSAN warnings in dw_mci_setup_bus().
>
> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1102:14
> shift exponent 250 is too large for 32-bit type 'unsigned int'
> Call trace:
> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> [<ffffff9008090c3c>] show_stack+0x14/0x20
> [<ffffff90087457b8>] dump_stack+0xe0/0x120
> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> [<ffffff9008d89cb8>] dw_mci_setup_bus+0x3a0/0x438
> [...]
>
> UBSAN: Undefined behaviour in drivers/mmc/host/dw_mmc.c:1132:27
> shift exponent 250 is too large for 32-bit type 'unsigned int'
> Call trace:
> [<ffffff90080908a8>] dump_backtrace+0x0/0x380
> [<ffffff9008090c3c>] show_stack+0x14/0x20
> [<ffffff90087457b8>] dump_stack+0xe0/0x120
> [<ffffff90087b1360>] ubsan_epilogue+0x18/0x68
> [<ffffff90087b1a94>] __ubsan_handle_shift_out_of_bounds+0x18c/0x1bc
> [<ffffff9008d89c9c>] dw_mci_setup_bus+0x384/0x438
> [...]
>
> The warnings are caused because of bit shift which is used to
> filter spamming message for CONFIG_MMC_CLKGATE, but the config is
> already removed. So this patch just removes the shift.
>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Applied this patch on my repository.
Thanks!
Best Regards,
Jaehoon Chung
> ---
> drivers/mmc/host/dw_mmc.c | 14 +++++---------
> drivers/mmc/host/dw_mmc.h | 4 ----
> 2 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 2cc6123..bada11e 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1099,12 +1099,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
> div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
>
> - if ((clock << div) != slot->__clk_old || force_clkinit)
> - dev_info(&slot->mmc->class_dev,
> - "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> - slot->id, host->bus_hz, clock,
> - div ? ((host->bus_hz / div) >> 1) :
> - host->bus_hz, div);
> + dev_info(&slot->mmc->class_dev,
> + "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> + slot->id, host->bus_hz, clock,
> + div ? ((host->bus_hz / div) >> 1) :
> + host->bus_hz, div);
>
> /* disable clock */
> mci_writel(host, CLKENA, 0);
> @@ -1127,9 +1126,6 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
> /* inform CIU */
> mci_send_cmd(slot, sdmmc_cmd_bits, 0);
> -
> - /* keep the clock with reflecting clock dividor */
> - slot->__clk_old = clock << div;
> }
>
> host->current_speed = clock;
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 1e8d838..5961037 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -245,9 +245,6 @@ extern int dw_mci_resume(struct dw_mci *host);
> * @queue_node: List node for placing this node in the @queue list of
> * &struct dw_mci.
> * @clock: Clock rate configured by set_ios(). Protected by host->lock.
> - * @__clk_old: The last updated clock with reflecting clock divider.
> - * Keeping track of this helps us to avoid spamming the console
> - * with CONFIG_MMC_CLKGATE.
> * @flags: Random state bits associated with the slot.
> * @id: Number of this slot.
> * @sdio_id: Number of this slot in the SDIO interrupt registers.
> @@ -262,7 +259,6 @@ struct dw_mci_slot {
> struct list_head queue_node;
>
> unsigned int clock;
> - unsigned int __clk_old;
>
> unsigned long flags;
> #define DW_MMC_CARD_PRESENT 0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-06-22 1:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20160608040654epcas1p2be228e3e02e35a640cb1e65228df79ad@epcas1p2.samsung.com>
2016-06-08 4:07 ` [PATCH] mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus() Seung-Woo Kim
2016-06-09 12:38 ` Jaehoon Chung
2016-06-10 1:29 ` Seung-Woo Kim
2016-06-17 1:30 ` Jaehoon Chung
2016-06-17 4:07 ` Seung-Woo Kim
2016-06-17 5:16 ` [PATCH v2] " Seung-Woo Kim
2016-06-20 2:34 ` Jaehoon Chung
2016-06-20 3:30 ` Seung-Woo Kim
2016-06-20 3:45 ` Seung-Woo Kim
2016-06-20 4:09 ` [PATCH v3] " Seung-Woo Kim
2016-06-22 1:27 ` Jaehoon Chung
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).