diff mbox series

[1/2] scsi: sg: use queue_logical_block_size() in max_sectors_bytes()

Message ID 20200923055248.1901-1-tom.ty89@gmail.com
State Rejected
Headers show
Series [1/2] scsi: sg: use queue_logical_block_size() in max_sectors_bytes() | expand

Commit Message

Tom Yan Sept. 23, 2020, 5:52 a.m. UTC
Logical block size was never / is no longer necessarily 512.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
 drivers/scsi/sg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Bart Van Assche Sept. 24, 2020, 5:52 p.m. UTC | #1
On 2020-09-22 22:52, Tom Yan wrote:
> Logical block size was never / is no longer necessarily 512.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
>  drivers/scsi/sg.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 20472aaaf630..8a2cca71017f 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -848,10 +848,11 @@ static int srp_done(Sg_fd *sfp, Sg_request *srp)
>  static int max_sectors_bytes(struct request_queue *q)
>  {
>  	unsigned int max_sectors = queue_max_sectors(q);
> +	max_sectors *= queue_logical_block_size(q);
>  
> -	max_sectors = min_t(unsigned int, max_sectors, INT_MAX >> 9);
> +	max_sectors = min_t(unsigned int, max_sectors, INT_MAX);
>  
> -	return max_sectors << 9;
> +	return max_sectors;
>  }

I think the above patch is wrong and also that it breaks code that is
correct. In the Linux kernel, "one sector" is by definition 512 bytes.
See also the definition of the SECTOR_SIZE and SECTOR_SHIFT constants.

Bart.
Tom Yan Sept. 28, 2020, 1:44 a.m. UTC | #2
Oh right, that's what logical_to_sectors()'s for. Thanks! :-)

On Fri, 25 Sep 2020 at 01:52, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2020-09-22 22:52, Tom Yan wrote:
> > Logical block size was never / is no longer necessarily 512.
> >
> > Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> > ---
> >  drivers/scsi/sg.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> > index 20472aaaf630..8a2cca71017f 100644
> > --- a/drivers/scsi/sg.c
> > +++ b/drivers/scsi/sg.c
> > @@ -848,10 +848,11 @@ static int srp_done(Sg_fd *sfp, Sg_request *srp)
> >  static int max_sectors_bytes(struct request_queue *q)
> >  {
> >       unsigned int max_sectors = queue_max_sectors(q);
> > +     max_sectors *= queue_logical_block_size(q);
> >
> > -     max_sectors = min_t(unsigned int, max_sectors, INT_MAX >> 9);
> > +     max_sectors = min_t(unsigned int, max_sectors, INT_MAX);
> >
> > -     return max_sectors << 9;
> > +     return max_sectors;
> >  }
>
> I think the above patch is wrong and also that it breaks code that is
> correct. In the Linux kernel, "one sector" is by definition 512 bytes.
> See also the definition of the SECTOR_SIZE and SECTOR_SHIFT constants.
>
> Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 20472aaaf630..8a2cca71017f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -848,10 +848,11 @@  static int srp_done(Sg_fd *sfp, Sg_request *srp)
 static int max_sectors_bytes(struct request_queue *q)
 {
 	unsigned int max_sectors = queue_max_sectors(q);
+	max_sectors *= queue_logical_block_size(q);
 
-	max_sectors = min_t(unsigned int, max_sectors, INT_MAX >> 9);
+	max_sectors = min_t(unsigned int, max_sectors, INT_MAX);
 
-	return max_sectors << 9;
+	return max_sectors;
 }
 
 static void