drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
Subsystems (2)
PATCH 1/1 drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
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.
--- drivers/target/target_core_alua.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/target/target_core_alua.c +2 -1
@@ -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) {
Signature · 1 line
1.7.11.7
Hi Chen,
Quoted 6 lines · Chen Gang
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!
Quoted 9 lines
Also need u64 type cast for u32 variable multiply u32 variable, or will cause type overflow issue.
Signed-off-by Chen Gang <[email protected]>--- 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
Quoted 14 lines
drivers/target/target_core_alua.c +2
-1@@ -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) {
Quoted 13 lines · Nicholas A. Bellinger
Hi Chen,
Quoted 10 lines · Chen Gang
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.
Quoted 9 lines
Quoted 5 lines
Signed-off-by Chen Gang <[email protected]>--- 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).
Quoted 2 lines
Regardless, thanks for your patch.
Thank you too.
Quoted 18 lines
--nab
Quoted 14 lines
drivers/target/target_core_alua.c +2
-1@@ -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) {
Signature · 3 lines
Chen Gang Open, share and attitude like air, water and life which God blessed
Quoted 24 lines · Chen Gang
Quoted 13 lines · Nicholas A. Bellinger
Hi Chen,
Quoted 10 lines · Chen Gang
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.
Quoted 9 lines
Quoted 5 lines
Signed-off-by Chen Gang <[email protected]>--- 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.
Quoted 9 lines
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
Quoted 19 lines
Quoted 17 lines
Quoted 15 lines
drivers/target/target_core_alua.c +2
-1@@ -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) {
Quoted 31 lines · Nicholas A. Bellinger
Quoted 30 lines · Chen Gang
Quoted 13 lines · Nicholas A. Bellinger
Hi Chen,
Quoted 10 lines · Chen Gang
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.
Quoted 9 lines
Quoted 5 lines
Signed-off-by Chen Gang <[email protected]>--- 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).
Quoted 7 lines
Quoted 3 lines
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. :-)
Quoted 2 lines
Hannes, can you please double check this below..?
Please help check when have time, thanks.
Quoted 25 lines
Thank you,
--nab
Quoted 19 lines
Quoted 17 lines
Quoted 15 lines
drivers/target/target_core_alua.c +2
-1@@ -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) {
Signature · 3 lines
Chen Gang Open, share and attitude like air, water and life which God blessed
Quoted 40 lines · Chen Gang
Quoted 31 lines · Nicholas A. Bellinger
Quoted 30 lines · Chen Gang
Quoted 13 lines · Nicholas A. Bellinger
Hi Chen,
Quoted 10 lines · Chen Gang
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.
Quoted 9 lines
Quoted 5 lines
Signed-off-by Chen Gang <[email protected]>--- 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).Quoted 7 lines
Quoted 3 lines
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.
Quoted 8 lines
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. :-)
Quoted 2 lines
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:
(quoted diff) +0 -0
b/drivers/target/target_core_alua
.c
index 9b1856d..54b1e52 100644
drivers/target/target_core_alua.c +1 -2
@@ -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
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 61 lines · Hannes Reinecke
Quoted 40 lines · Chen Gang
Quoted 31 lines · Nicholas A. Bellinger
Quoted 30 lines · Chen Gang
Quoted 13 lines · Nicholas A. Bellinger
Hi Chen,
Quoted 10 lines · Chen Gang
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.
Quoted 9 lines
Quoted 5 lines
Signed-off-by Chen Gang <[email protected]>--- 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).Quoted 7 lines
Quoted 3 lines
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.Quoted 8 lines
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. :-)
Quoted 2 lines
Hannes, can you please double check this below..?
Please help check when have time, thanks.
I would even convert
segment_sizeandsegment_multto u64, to ensure no overflows occur:drivers/target/target_core_alua renamed from drivers/target/target_core_alua.c +0
-0.c
index 9b1856d..54b1e52 100644
drivers/target/target_core_alua.c static inline int core_alua_state_lba_dependent( · line 483
Quoted 9 lines
drivers/target/target_core_alua.c +1
-2@@ -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.
Quoted 2 lines
Other than that the
sector_div()patch is correct.
So we really need use '/' instead of original '%'?
Thanks
Signature · 3 lines
Chen Gang Open, share and attitude like air, water and life which God blessed
Quoted 3 lines · Hannes Reinecke
Quoted 2 lines · Chen Gang
Quoted 1 line · Nicholas A. Bellinger
On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
<SNIP>
Quoted 32 lines
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
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.Quoted 8 lines
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. :-)
Quoted 2 lines
Hannes, can you please double check this below..?
Please help check when have time, thanks.
I would even convert
segment_sizeandsegment_multto u64, to ensure no overflows occur:drivers/target/target_core_alua renamed from drivers/target/target_core_alua.c +0
-0.c
index 9b1856d..54b1e52 100644
drivers/target/target_core_alua.c static inline int core_alua_state_lba_dependent( · line 483
Quoted 9 lines
drivers/target/target_core_alua.c +1
-2@@ -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..
Quoted 2 lines
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
Quoted 51 lines · Nicholas A. Bellinger
Quoted 3 lines · Hannes Reinecke
Quoted 2 lines · Chen Gang
Quoted 1 line · Nicholas A. Bellinger
On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
<SNIP>
Quoted 32 lines
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
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.Quoted 8 lines
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. :-)
Quoted 2 lines
Hannes, can you please double check this below..?
Please help check when have time, thanks.
I would even convert
segment_sizeandsegment_multto u64, to ensure no overflows occur:drivers/target/target_core_alua renamed from drivers/target/target_core_alua.c +0
-0.c
index 9b1856d..54b1e52 100644drivers/target/target_core_alua.c static inline int core_alua_state_lba_dependent( · line 483
Quoted 9 lines
drivers/target/target_core_alua.c +1
-2@@ -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..
Quoted 6 lines
Quoted 2 lines
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
drivers/target/target_core_alua.c +1 -2
@@ -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
Signature · 4 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +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)
Quoted 2 lines · Hannes Reinecke
Quoted 1 line · Nicholas A. Bellinger
On Wed, 2014-01-08 at 08:32 +0100, Hannes Reinecke wrote:
<SNIP>
Quoted 34 lines
Quoted 6 lines
Quoted 2 lines
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 bedrivers/target/target_core_alua.c +1
-2@@ -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.
Quoted 3 lines
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
Quoted 40 lines · Nicholas A. Bellinger
Quoted 2 lines · Hannes Reinecke
Quoted 1 line · Nicholas A. Bellinger
On Wed, 2014-01-08 at 08:32 +0100, Hannes Reinecke wrote:
<SNIP>
Quoted 34 lines
Quoted 6 lines
Quoted 2 lines
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 bedrivers/target/target_core_alua.c +1
-2@@ -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.
Quoted 8 lines
Quoted 3 lines
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.
Signature · 3 lines
Chen Gang Open, share and attitude like air, water and life which God blessed