* [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
@ 2013-12-21 2:08 Chen Gang
2013-12-22 2:56 ` Nicholas A. Bellinger
0 siblings, 1 reply; 12+ messages in thread
From: Chen Gang @ 2013-12-21 2:08 UTC (permalink / raw)
To: nab; +Cc: James Hogan, linux-scsi, target-devel, linux-kernel
In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
can not pass compiling (with allmodconfig under metag):
MODPOST 2909 modules
ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
Also need u64 type cast for u32 variable multiply u32 variable, or will
cause type overflow issue.
Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
drivers/target/target_core_alua.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index dc0d399..ff2aadc 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -489,7 +489,8 @@ static inline int core_alua_state_lba_dependent(
u64 first_lba = map->lba_map_first_lba;
if (segment_mult) {
- start_lba = lba % (segment_size * segment_mult);
+ u64 tmp = (u64)segment_size * segment_mult;
+ div64_u64_rem(lba, tmp, &start_lba);
last_lba = first_lba + segment_size - 1;
if (start_lba >= first_lba &&
start_lba <= last_lba) {
--
1.7.11.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2013-12-21 2:08 [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64 Chen Gang
@ 2013-12-22 2:56 ` Nicholas A. Bellinger
2013-12-22 9:17 ` Chen Gang
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2013-12-22 2:56 UTC (permalink / raw)
To: Chen Gang; +Cc: James Hogan, linux-scsi, target-devel, linux-kernel
Hi Chen,
On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
> In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
> can not pass compiling (with allmodconfig under metag):
>
> MODPOST 2909 modules
> ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
>
> Also need u64 type cast for u32 variable multiply u32 variable, or will
> cause type overflow issue.
>
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> drivers/target/target_core_alua.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
FYI, this unsigned long long division in core_alua_state_lba_dependent()
was fixed for 32-bit in linux-next >= 12192013 code.
Regardless, thanks for your patch.
--nab
> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> index dc0d399..ff2aadc 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -489,7 +489,8 @@ static inline int core_alua_state_lba_dependent(
> u64 first_lba = map->lba_map_first_lba;
>
> if (segment_mult) {
> - start_lba = lba % (segment_size * segment_mult);
> + u64 tmp = (u64)segment_size * segment_mult;
> + div64_u64_rem(lba, tmp, &start_lba);
> last_lba = first_lba + segment_size - 1;
> if (start_lba >= first_lba &&
> start_lba <= last_lba) {
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2013-12-22 2:56 ` Nicholas A. Bellinger
@ 2013-12-22 9:17 ` Chen Gang
2013-12-23 6:51 ` Nicholas A. Bellinger
0 siblings, 1 reply; 12+ messages in thread
From: Chen Gang @ 2013-12-22 9:17 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: James Hogan, linux-scsi, target-devel, linux-kernel, hare, Fengguang Wu
On 12/22/2013 10:56 AM, Nicholas A. Bellinger wrote:
> Hi Chen,
>
> On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
>> In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
>> can not pass compiling (with allmodconfig under metag):
>>
>> MODPOST 2909 modules
>> ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
>>
>> Also need u64 type cast for u32 variable multiply u32 variable, or will
>> cause type overflow issue.
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>> drivers/target/target_core_alua.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>
> FYI, this unsigned long long division in core_alua_state_lba_dependent()
> was fixed for 32-bit in linux-next >= 12192013 code.
>
OK, thanks.
The related fix patch changed "start_lba = lba % ..." to "start_lba =
lba / ...", and also assumed "segment_size * segment_mult" is still
within u32 (can not cause type over flow).
I guess the original author already knew about them, and intended to do
like that (if not, please let me know, thanks).
> Regardless, thanks for your patch.
>
Thank you too.
> --nab
>
>> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
>> index dc0d399..ff2aadc 100644
>> --- a/drivers/target/target_core_alua.c
>> +++ b/drivers/target/target_core_alua.c
>> @@ -489,7 +489,8 @@ static inline int core_alua_state_lba_dependent(
>> u64 first_lba = map->lba_map_first_lba;
>>
>> if (segment_mult) {
>> - start_lba = lba % (segment_size * segment_mult);
>> + u64 tmp = (u64)segment_size * segment_mult;
>> + div64_u64_rem(lba, tmp, &start_lba);
>> last_lba = first_lba + segment_size - 1;
>> if (start_lba >= first_lba &&
>> start_lba <= last_lba) {
>
>
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2013-12-22 9:17 ` Chen Gang
@ 2013-12-23 6:51 ` Nicholas A. Bellinger
2013-12-24 3:35 ` Chen Gang
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2013-12-23 6:51 UTC (permalink / raw)
To: Chen Gang
Cc: James Hogan, linux-scsi, target-devel, linux-kernel, hare, Fengguang Wu
On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
> On 12/22/2013 10:56 AM, Nicholas A. Bellinger wrote:
> > Hi Chen,
> >
> > On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
> >> In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
> >> can not pass compiling (with allmodconfig under metag):
> >>
> >> MODPOST 2909 modules
> >> ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
> >>
> >> Also need u64 type cast for u32 variable multiply u32 variable, or will
> >> cause type overflow issue.
> >>
> >>
> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> >> ---
> >> drivers/target/target_core_alua.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >
> > FYI, this unsigned long long division in core_alua_state_lba_dependent()
> > was fixed for 32-bit in linux-next >= 12192013 code.
> >
>
> OK, thanks.
>
> The related fix patch changed "start_lba = lba % ..." to "start_lba =
> lba / ...", and also assumed "segment_size * segment_mult" is still
> within u32 (can not cause type over flow).
>
> I guess the original author already knew about them, and intended to do
> like that (if not, please let me know, thanks).
>
Sorry, your correct that the original code is using modulo division to
calculate start_lba.
Hannes, can you please double check this below..?
Thank you,
--nab
> >
> >> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> >> index dc0d399..ff2aadc 100644
> >> --- a/drivers/target/target_core_alua.c
> >> +++ b/drivers/target/target_core_alua.c
> >> @@ -489,7 +489,8 @@ static inline int core_alua_state_lba_dependent(
> >> u64 first_lba = map->lba_map_first_lba;
> >>
> >> if (segment_mult) {
> >> - start_lba = lba % (segment_size * segment_mult);
> >> + u64 tmp = (u64)segment_size * segment_mult;
> >> + div64_u64_rem(lba, tmp, &start_lba);
> >> last_lba = first_lba + segment_size - 1;
> >> if (start_lba >= first_lba &&
> >> start_lba <= last_lba) {
> >
> >
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2013-12-23 6:51 ` Nicholas A. Bellinger
@ 2013-12-24 3:35 ` Chen Gang
2014-01-08 7:32 ` Hannes Reinecke
0 siblings, 1 reply; 12+ messages in thread
From: Chen Gang @ 2013-12-24 3:35 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: James Hogan, linux-scsi, target-devel, linux-kernel, hare, Fengguang Wu
On 12/23/2013 02:51 PM, Nicholas A. Bellinger wrote:
> On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
>> On 12/22/2013 10:56 AM, Nicholas A. Bellinger wrote:
>>> Hi Chen,
>>>
>>> On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
>>>> In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
>>>> can not pass compiling (with allmodconfig under metag):
>>>>
>>>> MODPOST 2909 modules
>>>> ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
>>>>
>>>> Also need u64 type cast for u32 variable multiply u32 variable, or will
>>>> cause type overflow issue.
>>>>
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>> ---
>>>> drivers/target/target_core_alua.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>
>>> FYI, this unsigned long long division in core_alua_state_lba_dependent()
>>> was fixed for 32-bit in linux-next >= 12192013 code.
>>>
>>
>> OK, thanks.
>>
>> The related fix patch changed "start_lba = lba % ..." to "start_lba =
>> lba / ...", and also assumed "segment_size * segment_mult" is still
>> within u32 (can not cause type over flow).
>>
>> I guess the original author already knew about them, and intended to do
>> like that (if not, please let me know, thanks).
>>
>
> Sorry, your correct that the original code is using modulo division to
> calculate start_lba.
>
Oh, that's all right, (in fact, don't need sorry), I am not quite
familiar with the details, so need related member help check it. :-)
> Hannes, can you please double check this below..?
>
Please help check when have time, thanks.
> Thank you,
>
> --nab
>
>>>
>>>> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
>>>> index dc0d399..ff2aadc 100644
>>>> --- a/drivers/target/target_core_alua.c
>>>> +++ b/drivers/target/target_core_alua.c
>>>> @@ -489,7 +489,8 @@ static inline int core_alua_state_lba_dependent(
>>>> u64 first_lba = map->lba_map_first_lba;
>>>>
>>>> if (segment_mult) {
>>>> - start_lba = lba % (segment_size * segment_mult);
>>>> + u64 tmp = (u64)segment_size * segment_mult;
>>>> + div64_u64_rem(lba, tmp, &start_lba);
>>>> last_lba = first_lba + segment_size - 1;
>>>> if (start_lba >= first_lba &&
>>>> start_lba <= last_lba) {
>>>
>>>
>>
>>
>
>
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2013-12-24 3:35 ` Chen Gang
@ 2014-01-08 7:32 ` Hannes Reinecke
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-08 7:32 UTC (permalink / raw)
To: Chen Gang, Nicholas A. Bellinger
Cc: James Hogan, linux-scsi, target-devel, linux-kernel, Fengguang Wu
On 12/24/2013 04:35 AM, Chen Gang wrote:
> On 12/23/2013 02:51 PM, Nicholas A. Bellinger wrote:
>> On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
>>> On 12/22/2013 10:56 AM, Nicholas A. Bellinger wrote:
>>>> Hi Chen,
>>>>
>>>> On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
>>>>> In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
>>>>> can not pass compiling (with allmodconfig under metag):
>>>>>
>>>>> MODPOST 2909 modules
>>>>> ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
>>>>>
>>>>> Also need u64 type cast for u32 variable multiply u32 variable, or will
>>>>> cause type overflow issue.
>>>>>
>>>>>
>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>> ---
>>>>> drivers/target/target_core_alua.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> FYI, this unsigned long long division in core_alua_state_lba_dependent()
>>>> was fixed for 32-bit in linux-next >= 12192013 code.
>>>>
>>>
>>> OK, thanks.
>>>
>>> The related fix patch changed "start_lba = lba % ..." to "start_lba =
>>> lba / ...", and also assumed "segment_size * segment_mult" is still
>>> within u32 (can not cause type over flow).
>>>
>>> I guess the original author already knew about them, and intended to do
>>> like that (if not, please let me know, thanks).
>>>
>>
>> Sorry, your correct that the original code is using modulo division to
>> calculate start_lba.
>>
>
> Oh, that's all right, (in fact, don't need sorry), I am not quite
> familiar with the details, so need related member help check it. :-)
>
>> Hannes, can you please double check this below..?
>>
>
> Please help check when have time, thanks.
>
I would even convert segment_size and segment_mult to u64,
to ensure no overflows occur:
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua
.c
index 9b1856d..54b1e52 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -477,8 +477,7 @@ static inline int core_alua_state_lba_dependent(
u8 *alua_ascq)
{
struct se_device *dev = cmd->se_dev;
- u32 segment_size, segment_mult, sectors;
- u64 lba;
+ u64 segment_size, segment_mult, sectors, lba;
/* Only need to check for cdb actually containing LBAs */
if (!(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB))
Other than that the sector_div() patch is correct.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
@ 2014-01-08 7:32 ` Hannes Reinecke
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-08 7:32 UTC (permalink / raw)
To: Chen Gang, Nicholas A. Bellinger
Cc: James Hogan, linux-scsi, target-devel, linux-kernel, Fengguang Wu
On 12/24/2013 04:35 AM, Chen Gang wrote:
> On 12/23/2013 02:51 PM, Nicholas A. Bellinger wrote:
>> On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
>>> On 12/22/2013 10:56 AM, Nicholas A. Bellinger wrote:
>>>> Hi Chen,
>>>>
>>>> On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
>>>>> In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
>>>>> can not pass compiling (with allmodconfig under metag):
>>>>>
>>>>> MODPOST 2909 modules
>>>>> ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
>>>>>
>>>>> Also need u64 type cast for u32 variable multiply u32 variable, or will
>>>>> cause type overflow issue.
>>>>>
>>>>>
>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>> ---
>>>>> drivers/target/target_core_alua.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> FYI, this unsigned long long division in core_alua_state_lba_dependent()
>>>> was fixed for 32-bit in linux-next >= 12192013 code.
>>>>
>>>
>>> OK, thanks.
>>>
>>> The related fix patch changed "start_lba = lba % ..." to "start_lba =
>>> lba / ...", and also assumed "segment_size * segment_mult" is still
>>> within u32 (can not cause type over flow).
>>>
>>> I guess the original author already knew about them, and intended to do
>>> like that (if not, please let me know, thanks).
>>>
>>
>> Sorry, your correct that the original code is using modulo division to
>> calculate start_lba.
>>
>
> Oh, that's all right, (in fact, don't need sorry), I am not quite
> familiar with the details, so need related member help check it. :-)
>
>> Hannes, can you please double check this below..?
>>
>
> Please help check when have time, thanks.
>
I would even convert segment_size and segment_mult to u64,
to ensure no overflows occur:
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua
.c
index 9b1856d..54b1e52 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -477,8 +477,7 @@ static inline int core_alua_state_lba_dependent(
u8 *alua_ascq)
{
struct se_device *dev = cmd->se_dev;
- u32 segment_size, segment_mult, sectors;
- u64 lba;
+ u64 segment_size, segment_mult, sectors, lba;
/* Only need to check for cdb actually containing LBAs */
if (!(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB))
Other than that the sector_div() patch is correct.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2014-01-08 7:32 ` Hannes Reinecke
(?)
@ 2014-01-08 14:40 ` Chen Gang
-1 siblings, 0 replies; 12+ messages in thread
From: Chen Gang @ 2014-01-08 14:40 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Nicholas A. Bellinger, James Hogan, linux-scsi, target-devel,
linux-kernel, Fengguang Wu
On 01/08/2014 03:32 PM, Hannes Reinecke wrote:
> On 12/24/2013 04:35 AM, Chen Gang wrote:
>> On 12/23/2013 02:51 PM, Nicholas A. Bellinger wrote:
>>> On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
>>>> On 12/22/2013 10:56 AM, Nicholas A. Bellinger wrote:
>>>>> Hi Chen,
>>>>>
>>>>> On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
>>>>>> In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
>>>>>> can not pass compiling (with allmodconfig under metag):
>>>>>>
>>>>>> MODPOST 2909 modules
>>>>>> ERROR: "__umoddi3" [drivers/target/target_core_mod.ko] undefined!
>>>>>>
>>>>>> Also need u64 type cast for u32 variable multiply u32 variable, or will
>>>>>> cause type overflow issue.
>>>>>>
>>>>>>
>>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>>> ---
>>>>>> drivers/target/target_core_alua.c | 3 ++-
>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>
>>>>> FYI, this unsigned long long division in core_alua_state_lba_dependent()
>>>>> was fixed for 32-bit in linux-next >= 12192013 code.
>>>>>
>>>>
>>>> OK, thanks.
>>>>
>>>> The related fix patch changed "start_lba = lba % ..." to "start_lba =
>>>> lba / ...", and also assumed "segment_size * segment_mult" is still
>>>> within u32 (can not cause type over flow).
>>>>
>>>> I guess the original author already knew about them, and intended to do
>>>> like that (if not, please let me know, thanks).
>>>>
>>>
>>> Sorry, your correct that the original code is using modulo division to
>>> calculate start_lba.
>>>
>>
>> Oh, that's all right, (in fact, don't need sorry), I am not quite
>> familiar with the details, so need related member help check it. :-)
>>
>>> Hannes, can you please double check this below..?
>>>
>>
>> Please help check when have time, thanks.
>>
> I would even convert segment_size and segment_mult to u64,
> to ensure no overflows occur:
>
> diff --git a/drivers/target/target_core_alua.c
> b/drivers/target/target_core_alua
> .c
> index 9b1856d..54b1e52 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -477,8 +477,7 @@ static inline int core_alua_state_lba_dependent(
> u8 *alua_ascq)
> {
> struct se_device *dev = cmd->se_dev;
> - u32 segment_size, segment_mult, sectors;
> - u64 lba;
> + u64 segment_size, segment_mult, sectors, lba;
>
> /* Only need to check for cdb actually containing LBAs */
> if (!(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB))
>
>
OK, thanks.
> Other than that the sector_div() patch is correct.
>
So we really need use '/' instead of original '%'?
Thanks
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2014-01-08 7:32 ` Hannes Reinecke
(?)
(?)
@ 2014-01-08 23:18 ` Nicholas A. Bellinger
2014-01-09 10:17 ` Hannes Reinecke
-1 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-01-08 23:18 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Chen Gang, James Hogan, linux-scsi, target-devel, linux-kernel,
Fengguang Wu
On Wed, 2014-01-08 at 08:32 +0100, Hannes Reinecke wrote:
> On 12/24/2013 04:35 AM, Chen Gang wrote:
> > On 12/23/2013 02:51 PM, Nicholas A. Bellinger wrote:
> >> On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
<SNIP>
> >>> The related fix patch changed "start_lba = lba % ..." to "start_lba =
> >>> lba / ...", and also assumed "segment_size * segment_mult" is still
> >>> within u32 (can not cause type over flow).
> >>>
> >>> I guess the original author already knew about them, and intended to do
> >>> like that (if not, please let me know, thanks).
> >>>
> >>
> >> Sorry, your correct that the original code is using modulo division to
> >> calculate start_lba.
> >>
> >
> > Oh, that's all right, (in fact, don't need sorry), I am not quite
> > familiar with the details, so need related member help check it. :-)
> >
> >> Hannes, can you please double check this below..?
> >>
> >
> > Please help check when have time, thanks.
> >
> I would even convert segment_size and segment_mult to u64,
> to ensure no overflows occur:
>
> diff --git a/drivers/target/target_core_alua.c
> b/drivers/target/target_core_alua
> .c
> index 9b1856d..54b1e52 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -477,8 +477,7 @@ static inline int core_alua_state_lba_dependent(
> u8 *alua_ascq)
> {
> struct se_device *dev = cmd->se_dev;
> - u32 segment_size, segment_mult, sectors;
> - u64 lba;
> + u64 segment_size, segment_mult, sectors, lba;
>
> /* Only need to check for cdb actually containing LBAs */
> if (!(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB))
>
>
Will squash the above into the original patch shortly in for-next..
> Other than that the sector_div() patch is correct.
>
<nod> Thanks for confirming that sector_div() is correct here vs. the
original code using modulo that Chen had pointed out.
Thanks Hannes!
--nab
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2014-01-08 23:18 ` Nicholas A. Bellinger
@ 2014-01-09 10:17 ` Hannes Reinecke
2014-01-10 5:47 ` Nicholas A. Bellinger
0 siblings, 1 reply; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-09 10:17 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Chen Gang, James Hogan, linux-scsi, target-devel, linux-kernel,
Fengguang Wu
On 01/09/2014 12:18 AM, Nicholas A. Bellinger wrote:
> On Wed, 2014-01-08 at 08:32 +0100, Hannes Reinecke wrote:
>> On 12/24/2013 04:35 AM, Chen Gang wrote:
>>> On 12/23/2013 02:51 PM, Nicholas A. Bellinger wrote:
>>>> On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
>
> <SNIP>
>
>>>>> The related fix patch changed "start_lba = lba % ..." to "start_lba =
>>>>> lba / ...", and also assumed "segment_size * segment_mult" is still
>>>>> within u32 (can not cause type over flow).
>>>>>
>>>>> I guess the original author already knew about them, and intended to do
>>>>> like that (if not, please let me know, thanks).
>>>>>
>>>>
>>>> Sorry, your correct that the original code is using modulo division to
>>>> calculate start_lba.
>>>>
>>>
>>> Oh, that's all right, (in fact, don't need sorry), I am not quite
>>> familiar with the details, so need related member help check it. :-)
>>>
>>>> Hannes, can you please double check this below..?
>>>>
>>>
>>> Please help check when have time, thanks.
>>>
>> I would even convert segment_size and segment_mult to u64,
>> to ensure no overflows occur:
>>
>> diff --git a/drivers/target/target_core_alua.c
>> b/drivers/target/target_core_alua
>> .c
>> index 9b1856d..54b1e52 100644
>> --- a/drivers/target/target_core_alua.c
>> +++ b/drivers/target/target_core_alua.c
>> @@ -477,8 +477,7 @@ static inline int core_alua_state_lba_dependent(
>> u8 *alua_ascq)
>> {
>> struct se_device *dev = cmd->se_dev;
>> - u32 segment_size, segment_mult, sectors;
>> - u64 lba;
>> + u64 segment_size, segment_mult, sectors, lba;
>>
>> /* Only need to check for cdb actually containing LBAs */
>> if (!(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB))
>>
>>
>
> Will squash the above into the original patch shortly in for-next..
>
>> Other than that the sector_div() patch is correct.
>>
>
> <nod> Thanks for confirming that sector_div() is correct here vs. the
> original code using modulo that Chen had pointed out.
>
Ah, _that_ was the issue.
I was wondering why you kept on poking me ...
Well.
No, that's actually _not_ correct.
The correct fix would be
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua.c
index 54b1e52..12da9b3 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -500,8 +500,7 @@ static inline int core_alua_state_lba_dependent(
if (segment_mult) {
u64 tmp = lba;
- sector_div(tmp, segment_size *
segment_mult);
- start_lba = tmp;
+ start_lba = sector_div(tmp, segment_size
* segment_mult);
last_lba = first_lba + segment_size - 1;
if (start_lba >= first_lba &&
(beware of line breaks ...)
Thing is, we need to calculate the offset into the segment to figure out
which map entry to use.
The actual number of the segment (as had been calculated with the
original fix) is immaterial here.
Sorry for this. The email thread just flew past me during Xmas
with me not paying real attention.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2014-01-09 10:17 ` Hannes Reinecke
@ 2014-01-10 5:47 ` Nicholas A. Bellinger
2014-01-10 16:06 ` Chen Gang
0 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2014-01-10 5:47 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Chen Gang, James Hogan, linux-scsi, target-devel, linux-kernel,
Fengguang Wu
On Thu, 2014-01-09 at 11:17 +0100, Hannes Reinecke wrote:
> On 01/09/2014 12:18 AM, Nicholas A. Bellinger wrote:
> > On Wed, 2014-01-08 at 08:32 +0100, Hannes Reinecke wrote:
<SNIP>
> >> Other than that the sector_div() patch is correct.
> >>
> >
> > <nod> Thanks for confirming that sector_div() is correct here vs. the
> > original code using modulo that Chen had pointed out.
> >
> Ah, _that_ was the issue.
> I was wondering why you kept on poking me ...
>
> Well.
> No, that's actually _not_ correct.
> The correct fix would be
>
> diff --git a/drivers/target/target_core_alua.c
> b/drivers/target/target_core_alua.c
> index 54b1e52..12da9b3 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -500,8 +500,7 @@ static inline int core_alua_state_lba_dependent(
>
> if (segment_mult) {
> u64 tmp = lba;
> - sector_div(tmp, segment_size * segment_mult);
> - start_lba = tmp;
> + start_lba = sector_div(tmp, segment_size * segment_mult);
>
> last_lba = first_lba + segment_size - 1;
> if (start_lba >= first_lba &&
> (beware of line breaks ...)
> Thing is, we need to calculate the offset into the segment to figure out
> which map entry to use.
> The actual number of the segment (as had been calculated with the
> original fix) is immaterial here.
>
> Sorry for this. The email thread just flew past me during Xmas
> with me not paying real attention.
>
Applied + squashed. Apologies for the initial pre-holiday BUG..
Thanks Hannes!
--nab
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
2014-01-10 5:47 ` Nicholas A. Bellinger
@ 2014-01-10 16:06 ` Chen Gang
0 siblings, 0 replies; 12+ messages in thread
From: Chen Gang @ 2014-01-10 16:06 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Hannes Reinecke, James Hogan, linux-scsi, target-devel,
linux-kernel, Fengguang Wu
On 01/10/2014 01:47 PM, Nicholas A. Bellinger wrote:
> On Thu, 2014-01-09 at 11:17 +0100, Hannes Reinecke wrote:
>> On 01/09/2014 12:18 AM, Nicholas A. Bellinger wrote:
>>> On Wed, 2014-01-08 at 08:32 +0100, Hannes Reinecke wrote:
>
> <SNIP>
>
>>>> Other than that the sector_div() patch is correct.
>>>>
>>>
>>> <nod> Thanks for confirming that sector_div() is correct here vs. the
>>> original code using modulo that Chen had pointed out.
>>>
>> Ah, _that_ was the issue.
>> I was wondering why you kept on poking me ...
>>
>> Well.
>> No, that's actually _not_ correct.
>> The correct fix would be
>>
>> diff --git a/drivers/target/target_core_alua.c
>> b/drivers/target/target_core_alua.c
>> index 54b1e52..12da9b3 100644
>> --- a/drivers/target/target_core_alua.c
>> +++ b/drivers/target/target_core_alua.c
>> @@ -500,8 +500,7 @@ static inline int core_alua_state_lba_dependent(
>>
>> if (segment_mult) {
>> u64 tmp = lba;
>> - sector_div(tmp, segment_size * segment_mult);
>> - start_lba = tmp;
>> + start_lba = sector_div(tmp, segment_size * segment_mult);
>>
>> last_lba = first_lba + segment_size - 1;
>> if (start_lba >= first_lba &&
>> (beware of line breaks ...)
>> Thing is, we need to calculate the offset into the segment to figure out
>> which map entry to use.
>> The actual number of the segment (as had been calculated with the
>> original fix) is immaterial here.
>>
>> Sorry for this. The email thread just flew past me during Xmas
>> with me not paying real attention.
>>
>
> Applied + squashed. Apologies for the initial pre-holiday BUG..
>
> Thanks Hannes!
>
Thank all of you.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-01-10 16:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-21 2:08 [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64 Chen Gang
2013-12-22 2:56 ` Nicholas A. Bellinger
2013-12-22 9:17 ` Chen Gang
2013-12-23 6:51 ` Nicholas A. Bellinger
2013-12-24 3:35 ` Chen Gang
2014-01-08 7:32 ` Hannes Reinecke
2014-01-08 7:32 ` Hannes Reinecke
2014-01-08 14:40 ` Chen Gang
2014-01-08 23:18 ` Nicholas A. Bellinger
2014-01-09 10:17 ` Hannes Reinecke
2014-01-10 5:47 ` Nicholas A. Bellinger
2014-01-10 16:06 ` Chen Gang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.