[PATCH alternative] [SCSI] aic7xxx: Fix warnings with CONFIG_PM=n
Fixes the following warnings:
CC [M] drivers/scsi/aic7xxx/aic79xx_osm_pci.o
drivers/scsi/aic7xxx/aic79xx_osm_pci.c:101: warning:
‘ahd_linux_pci_dev_suspend’ defined but not used
drivers/scsi/aic7xxx/aic79xx_osm_pci.c:121: warning: ‘ahd_linux_pci_dev_resume’
defined but not used
CC [M] drivers/scsi/aic7xxx/aic7xxx_osm_pci.o
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:148: warning:
‘ahc_linux_pci_dev_suspend’ defined but not used
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:166: warning: ‘ahc_linux_pci_dev_resume’
defined but not used
Reorder code as suggested by Randy Dunlap to minimize
needed #ifdefs
Cc: Randy Dunlap <[EMAIL PROTECTED]>
Signed-off-by: Frank Lichtenheld <[EMAIL PROTECTED]>
---
drivers/scsi/aic7xxx/aic79xx_osm_pci.c | 25 +
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c | 25 +
2 files changed, 26 insertions(+), 24 deletions(-)
> But "ideally" the functions would be defined before the
> struct pci_driver data, so the prototypes for them could be removed.
Hmm, I'm a bit sceptical about the reordering, but here you go.
Choose whatever you like.
diff --git a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
index 66f0259..f6dbd3e 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
@@ -50,8 +50,6 @@ static intahd_linux_pci_reserve_io_regions(struct
ahd_softc *ahd,
static int ahd_linux_pci_reserve_mem_region(struct ahd_softc *ahd,
u_long *bus_addr,
uint8_t __iomem **maddr);
-static int ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t
mesg);
-static int ahd_linux_pci_dev_resume(struct pci_dev *pdev);
static voidahd_linux_pci_dev_remove(struct pci_dev *pdev);
/* Define the macro locally since it's different for different class of chips.
@@ -85,17 +83,7 @@ static struct pci_device_id ahd_linux_pci_id_table[] = {
MODULE_DEVICE_TABLE(pci, ahd_linux_pci_id_table);
-static struct pci_driver aic79xx_pci_driver = {
- .name = "aic79xx",
- .probe = ahd_linux_pci_dev_probe,
#ifdef CONFIG_PM
- .suspend= ahd_linux_pci_dev_suspend,
- .resume = ahd_linux_pci_dev_resume,
-#endif
- .remove = ahd_linux_pci_dev_remove,
- .id_table = ahd_linux_pci_id_table
-};
-
static int
ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t mesg)
{
@@ -139,6 +127,19 @@ ahd_linux_pci_dev_resume(struct pci_dev *pdev)
return rc;
}
+#else
+#define ahd_linux_pci_dev_suspend NULL
+#define ahd_linux_pci_dev_resume NULL
+#endif
+
+static struct pci_driver aic79xx_pci_driver = {
+ .name = "aic79xx",
+ .probe = ahd_linux_pci_dev_probe,
+ .suspend= ahd_linux_pci_dev_suspend,
+ .resume = ahd_linux_pci_dev_resume,
+ .remove = ahd_linux_pci_dev_remove,
+ .id_table = ahd_linux_pci_id_table
+};
static void
ahd_linux_pci_dev_remove(struct pci_dev *pdev)
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
index 4488946..ab53189 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -49,8 +49,6 @@ static intahc_linux_pci_reserve_io_region(struct
ahc_softc *ahc,
static int ahc_linux_pci_reserve_mem_region(struct ahc_softc *ahc,
u_long *bus_addr,
uint8_t __iomem **maddr);
-static int ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t
mesg);
-static int ahc_linux_pci_dev_resume(struct pci_dev *pdev);
static voidahc_linux_pci_dev_remove(struct pci_dev *pdev);
/* Define the macro locally since it's different for different class of chips.
@@ -132,17 +130,7 @@ static struct pci_device_id ahc_linux_pci_id_table[] = {
MODULE_DEVICE_TABLE(pci, ahc_linux_pci_id_table);
-static struct pci_driver aic7xxx_pci_driver = {
- .name = "aic7xxx",
- .probe = ahc_linux_pci_dev_probe,
#ifdef CONFIG_PM
- .suspend= ahc_linux_pci_dev_suspend,
- .resume = ahc_linux_pci_dev_resume,
-#endif
- .remove = ahc_linux_pci_dev_remove,
- .id_table = ahc_linux_pci_id_table
-};
-
static int
ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t mesg)
{
@@ -182,6 +170,19 @@ ahc_linux_pci_dev_resume(struct pci_dev *pdev)
return (ahc_resume(ahc));
}
+#else
+#define ahc_linux_pci_dev_suspend NULL
+#define ahc_linux_pci_dev_resume NULL
+#endif
+
+st
Re: [PATCH] [SCSI] aic7xxx: Fix warnings with CONFIG_PM=n
Frank Lichtenheld wrote: On Thu, Nov 08, 2007 at 03:58:34PM -0800, Randy Dunlap wrote: Hi, The preferred method of fixing this type of warning is to (warning, not a full patch here): a. change the struct pci_driver not to use #ifdef CONFIG_PM/#endif; instead, it always says: .suspend= ahd_linux_pci_dev_suspend, .resume = ahd_linux_pci_dev_resume, and those pointers are built depending on CONFIG_PM like so: #ifdef CONFIG_PM ... functions as they are now ... #else #define ahd_linux_pci_dev_suspend NULL #define ahd_linux_pci_dev_resumeNULL #endif so the ifdef/endif blocks are localized to one place in each driver. Hmm, technically _two_ places since you still need them around both declaration and definition of the functions, right? OK, that's the case without any code movement. But "ideally" the functions would be defined before the struct pci_driver data, so the prototypes for them could be removed. -- ~Randy - 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
Re: [PATCH] [SCSI] aic7xxx: Fix warnings with CONFIG_PM=n
On Fri, 9 Nov 2007 00:03:48 +0100 Frank Lichtenheld wrote:
> Fixes the following warnings:
> CC [M] drivers/scsi/aic7xxx/aic79xx_osm_pci.o
> drivers/scsi/aic7xxx/aic79xx_osm_pci.c:101: warning:
> ‘ahd_linux_pci_dev_suspend’ defined but not used
> drivers/scsi/aic7xxx/aic79xx_osm_pci.c:121: warning:
> ‘ahd_linux_pci_dev_resume’ defined but not used
>
> CC [M] drivers/scsi/aic7xxx/aic7xxx_osm_pci.o
> drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:148: warning:
> ‘ahc_linux_pci_dev_suspend’ defined but not used
> drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:166: warning:
> ‘ahc_linux_pci_dev_resume’ defined but not used
>
> Signed-off-by: Frank Lichtenheld <[EMAIL PROTECTED]>
> ---
> drivers/scsi/aic7xxx/aic79xx_osm_pci.c |4
> drivers/scsi/aic7xxx/aic7xxx_osm_pci.c |4
> 2 files changed, 8 insertions(+), 0 deletions(-)
Hi,
The preferred method of fixing this type of warning is to
(warning, not a full patch here):
a. change the struct pci_driver not to use #ifdef CONFIG_PM/#endif;
instead, it always says:
.suspend= ahd_linux_pci_dev_suspend,
.resume = ahd_linux_pci_dev_resume,
and those pointers are built depending on CONFIG_PM like so:
#ifdef CONFIG_PM
... functions as they are now ...
#else
#define ahd_linux_pci_dev_suspend NULL
#define ahd_linux_pci_dev_resumeNULL
#endif
so the ifdef/endif blocks are localized to one place in each driver.
> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> index 66f0259..36d7618 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> @@ -50,8 +50,10 @@ static int ahd_linux_pci_reserve_io_regions(struct
> ahd_softc *ahd,
> static int ahd_linux_pci_reserve_mem_region(struct ahd_softc *ahd,
>u_long *bus_addr,
>uint8_t __iomem **maddr);
> +#ifdef CONFIG_PM
> static int ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t
> mesg);
> static int ahd_linux_pci_dev_resume(struct pci_dev *pdev);
> +#endif
> static void ahd_linux_pci_dev_remove(struct pci_dev *pdev);
>
> /* Define the macro locally since it's different for different class of
> chips.
> @@ -96,6 +98,7 @@ static struct pci_driver aic79xx_pci_driver = {
> .id_table = ahd_linux_pci_id_table
> };
>
> +#ifdef CONFIG_PM
> static int
> ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t mesg)
> {
> @@ -139,6 +142,7 @@ ahd_linux_pci_dev_resume(struct pci_dev *pdev)
>
> return rc;
> }
> +#endif
>
> static void
> ahd_linux_pci_dev_remove(struct pci_dev *pdev)
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
> b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
> index 4488946..212c2c8 100644
> --- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
> +++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
> @@ -49,8 +49,10 @@ static int ahc_linux_pci_reserve_io_region(struct
> ahc_softc *ahc,
> static int ahc_linux_pci_reserve_mem_region(struct ahc_softc *ahc,
>u_long *bus_addr,
>uint8_t __iomem **maddr);
> +#ifdef CONFIG_PM
> static int ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t
> mesg);
> static int ahc_linux_pci_dev_resume(struct pci_dev *pdev);
> +#endif
> static void ahc_linux_pci_dev_remove(struct pci_dev *pdev);
>
> /* Define the macro locally since it's different for different class of
> chips.
> @@ -143,6 +145,7 @@ static struct pci_driver aic7xxx_pci_driver = {
> .id_table = ahc_linux_pci_id_table
> };
>
> +#ifdef CONFIG_PM
> static int
> ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t mesg)
> {
> @@ -182,6 +185,7 @@ ahc_linux_pci_dev_resume(struct pci_dev *pdev)
>
> return (ahc_resume(ahc));
> }
> +#endif
>
> static void
> ahc_linux_pci_dev_remove(struct pci_dev *pdev)
> --
---
~Randy
-
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
Re: [PATCH] [SCSI] aic7xxx: Fix warnings with CONFIG_PM=n
On Thu, Nov 08, 2007 at 03:58:34PM -0800, Randy Dunlap wrote: > Hi, > The preferred method of fixing this type of warning is to > (warning, not a full patch here): > > a. change the struct pci_driver not to use #ifdef CONFIG_PM/#endif; > instead, it always says: > > .suspend= ahd_linux_pci_dev_suspend, > .resume = ahd_linux_pci_dev_resume, > > and those pointers are built depending on CONFIG_PM like so: > > #ifdef CONFIG_PM > ... functions as they are now ... > #else > #define ahd_linux_pci_dev_suspend NULL > #define ahd_linux_pci_dev_resume NULL > #endif > > so the ifdef/endif blocks are localized to one place in each driver. Hmm, technically _two_ places since you still need them around both declaration and definition of the functions, right? Gruesse, -- Frank Lichtenheld <[EMAIL PROTECTED]> www: http://www.djpig.de/ - 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
[PATCH] [SCSI] aic7xxx: Fix warnings with CONFIG_PM=n
Fixes the following warnings:
CC [M] drivers/scsi/aic7xxx/aic79xx_osm_pci.o
drivers/scsi/aic7xxx/aic79xx_osm_pci.c:101: warning:
‘ahd_linux_pci_dev_suspend’ defined but not used
drivers/scsi/aic7xxx/aic79xx_osm_pci.c:121: warning: ‘ahd_linux_pci_dev_resume’
defined but not used
CC [M] drivers/scsi/aic7xxx/aic7xxx_osm_pci.o
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:148: warning:
‘ahc_linux_pci_dev_suspend’ defined but not used
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:166: warning: ‘ahc_linux_pci_dev_resume’
defined but not used
Signed-off-by: Frank Lichtenheld <[EMAIL PROTECTED]>
---
drivers/scsi/aic7xxx/aic79xx_osm_pci.c |4
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c |4
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
index 66f0259..36d7618 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
@@ -50,8 +50,10 @@ static int ahd_linux_pci_reserve_io_regions(struct
ahd_softc *ahd,
static int ahd_linux_pci_reserve_mem_region(struct ahd_softc *ahd,
u_long *bus_addr,
uint8_t __iomem **maddr);
+#ifdef CONFIG_PM
static int ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t
mesg);
static int ahd_linux_pci_dev_resume(struct pci_dev *pdev);
+#endif
static voidahd_linux_pci_dev_remove(struct pci_dev *pdev);
/* Define the macro locally since it's different for different class of chips.
@@ -96,6 +98,7 @@ static struct pci_driver aic79xx_pci_driver = {
.id_table = ahd_linux_pci_id_table
};
+#ifdef CONFIG_PM
static int
ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t mesg)
{
@@ -139,6 +142,7 @@ ahd_linux_pci_dev_resume(struct pci_dev *pdev)
return rc;
}
+#endif
static void
ahd_linux_pci_dev_remove(struct pci_dev *pdev)
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
index 4488946..212c2c8 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -49,8 +49,10 @@ static int ahc_linux_pci_reserve_io_region(struct
ahc_softc *ahc,
static int ahc_linux_pci_reserve_mem_region(struct ahc_softc *ahc,
u_long *bus_addr,
uint8_t __iomem **maddr);
+#ifdef CONFIG_PM
static int ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t
mesg);
static int ahc_linux_pci_dev_resume(struct pci_dev *pdev);
+#endif
static voidahc_linux_pci_dev_remove(struct pci_dev *pdev);
/* Define the macro locally since it's different for different class of chips.
@@ -143,6 +145,7 @@ static struct pci_driver aic7xxx_pci_driver = {
.id_table = ahc_linux_pci_id_table
};
+#ifdef CONFIG_PM
static int
ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t mesg)
{
@@ -182,6 +185,7 @@ ahc_linux_pci_dev_resume(struct pci_dev *pdev)
return (ahc_resume(ahc));
}
+#endif
static void
ahc_linux_pci_dev_remove(struct pci_dev *pdev)
--
1.5.3.4
-
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
Re: [PATCH] Add Documentation/DocBook/scsi_midlayer.tmpl and update kerneldoc comments and Makefile.
On Wednesday 07 November 2007 00:47:20 James Bottomley wrote: > On Sun, 2007-11-04 at 10:02 -0800, Randy Dunlap wrote: > > My only concern with this ATM is its name (SCSI midlayer), but > > chapter 2 is SCSI upper layer, chap 3 is SCSI midlayer, and > > chap 4 is SCSI low-level interfaces. > > > > Chapters 2 & 4 are mostly stubs or todo list, which is OK, but if > > those chapters remain in this docbook, it looks like the docbook > > name should be changed (maybe later). > > How about just scsi.tmpl then? I have no attachment to a specific name. Call it what you like. :) Rob -- "One of my most productive days was throwing away 1000 lines of code." - Ken Thompson. - 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
Re: [linux-usb-devel] NEXX NF-315: unusual_devs.h entry submission
(CC'ing linux-scsi on this) On Thu, Nov 08, 2007 at 01:28:53AM +0300, Serge (gentoosiast) Koksharov wrote: > On Wed, Nov 07, 2007 at 02:31:45PM -0500, Alan Stern wrote: > > This suggests that the device needs a longer timeout for the INQUIRY > > command. The default timeout is 5 seconds (the driver adds an extra > > 0.5 seconds for some reason), but it is a module parameter you can > > adjust. Try adding > > > > options sd_mod inq_timeout=10 > > > > to your /etc/modprobe.conf. This might fix the problem. > > Yes! Your suggestion solved my problem, thank you very much. What we have here is a device which takes a particularly long time to respond to an INQUIRY command (a little over 6 seconds, I think). Would there be any objections to permanently modifying sd_mod to change the default INQUIRY timeout to 10 seconds (currently 5)? Matt -- Matthew Dharm Home: [EMAIL PROTECTED] Maintainer, Linux USB Mass Storage Driver G: Baaap booop BAHHHP. Mir: 9600 Baud? Mik: No, no! 9600 goes baap booop, not booop bahhhp! -- Greg, Miranda and Mike User Friendly, 12/31/1998 pgpuV5SMUfqiG.pgp Description: PGP signature
Re: [PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects
On Thu, Nov 08, 2007 at 01:11:01PM -0500, Tony Battersby wrote: > HBA is inside an embedded device sold by my company, and customers can > attach their own SCSI devices to it. There is one type of SCSI device > that we support which doesn't work with disconnect privilege enabled, so > my code running in the embedded device needs to be able to > enable/disable disconnect privilege dynamically based on whether or not > the customer has attached this specific broken device. > > It would be nice to have a generic way to control it. That would save > me the bother of sending my patch to control disconnect privilege for > LSI MPT Fusion Ultra320 HBAs via mptctl. ;-) Yes, the embedded space is a good enough reason to implement it in the SPI transport class. And I think we should take that as an opportunity to kill the sym2 /proc support or at least the write support. - 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
Re: [PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects
James Bottomley wrote: > On Wed, 2007-11-07 at 15:37 -0500, Tony Battersby wrote: > >> This patch fixes the sym53c8xx "setflag" user command to control >> disconnect privilege, which has been broken for a long time. >> > > The first observation is that the sym specific setflags interface is > being replaced by the parallel transport specific sysfs interface. > However, as you've probably noticed, the SPI transport has no flag for > disconnect permission, so how important actually is this? If it's > important, then we can put it in scsi_transport_spi ... if not, then we > should probably just eliminate it from sym2. > > James Most people would probably just use the HBA's BIOS utility to configure the NVRAM to disallow disconnects if needed. However, in my case, the HBA is inside an embedded device sold by my company, and customers can attach their own SCSI devices to it. There is one type of SCSI device that we support which doesn't work with disconnect privilege enabled, so my code running in the embedded device needs to be able to enable/disable disconnect privilege dynamically based on whether or not the customer has attached this specific broken device. It would be nice to have a generic way to control it. That would save me the bother of sending my patch to control disconnect privilege for LSI MPT Fusion Ultra320 HBAs via mptctl. ;-) Tony - 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
RE: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8
Resounding ACK. I just finished *exactly* the same set of changes, composed the patch and was about to hit send when this one came over the wire from you! There was absolutely no differences between our patches (save for the fact I did not place the AIF ones in as they are already in the queue, one is already on -mm). I am going to return to this at some future date and figure out the problems surrounding the context imbalances that are present, making code that determines which context it is called from (sysfs, error recovery or from the background thread) and plays with the various locks confuses sparse. Rewriting so that the contexts are less programmatic is in order... Sincerely -- Mark Salyzyn > -Original Message- > From: Christoph Hellwig [mailto:[EMAIL PROTECTED] > Sent: Thursday, November 08, 2007 12:28 PM > To: Salyzyn, Mark > Cc: Christoph Hellwig; Andreas Schwab; Stephen Rothwell; > linux-scsi@vger.kernel.org; LKML > Subject: Re: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8 > > On Wed, Nov 07, 2007 at 01:51:44PM -0500, Salyzyn, Mark wrote: > > Christoph Hellwig [mailto:[EMAIL PROTECTED] sez: > > > Did anyone run the driver through sparse to see if we have > > > more issues like this? > > > > There are some warnings from sparse, none like this one. I will deal > > with the warnings ... > > Actually there are a lot of endianess warnings, fortunately > most of them > harmless. The patch below fixes all of them up (including the ones in > the patch I replied to), except for aac_init_adapter which is > really odd > and I don't know what to do. > - 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
Re: [PATCH] [SCSI] sym53c8xx: fix setflag user command to control disconnects
On Wed, 2007-11-07 at 15:37 -0500, Tony Battersby wrote: > This patch fixes the sym53c8xx "setflag" user command to control > disconnect privilege, which has been broken for a long time. The first observation is that the sym specific setflags interface is being replaced by the parallel transport specific sysfs interface. However, as you've probably noticed, the SPI transport has no flag for disconnect permission, so how important actually is this? If it's important, then we can put it in scsi_transport_spi ... if not, then we should probably just eliminate it from sym2. James - 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
Re: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8
On Wed, Nov 07, 2007 at 01:51:44PM -0500, Salyzyn, Mark wrote:
> Christoph Hellwig [mailto:[EMAIL PROTECTED] sez:
> > Did anyone run the driver through sparse to see if we have
> > more issues like this?
>
> There are some warnings from sparse, none like this one. I will deal
> with the warnings ...
Actually there are a lot of endianess warnings, fortunately most of them
harmless. The patch below fixes all of them up (including the ones in
the patch I replied to), except for aac_init_adapter which is really odd
and I don't know what to do.
Signed-off-by: Christoph Hellwig <[EMAIL PROTECTED]>
Index: linux-2.6/drivers/scsi/aacraid/aachba.c
===
--- linux-2.6.orig/drivers/scsi/aacraid/aachba.c2007-11-08
17:09:50.0 +0100
+++ linux-2.6/drivers/scsi/aacraid/aachba.c 2007-11-08 17:14:43.0
+0100
@@ -981,7 +981,7 @@
aac_fib_init(fib);
readcmd = (struct aac_read *) fib_data(fib);
readcmd->command = cpu_to_le32(VM_CtBlockRead);
- readcmd->cid = cpu_to_le16(scmd_id(cmd));
+ readcmd->cid = cpu_to_le32(scmd_id(cmd));
readcmd->block = cpu_to_le32((u32)(lba&0x));
readcmd->count = cpu_to_le32(count * 512);
@@ -1072,7 +1072,7 @@
aac_fib_init(fib);
writecmd = (struct aac_write *) fib_data(fib);
writecmd->command = cpu_to_le32(VM_CtBlockWrite);
- writecmd->cid = cpu_to_le16(scmd_id(cmd));
+ writecmd->cid = cpu_to_le32(scmd_id(cmd));
writecmd->block = cpu_to_le32((u32)(lba&0x));
writecmd->count = cpu_to_le32(count * 512);
writecmd->sg.count = cpu_to_le32(1);
@@ -1306,8 +1306,8 @@
dev->supplement_adapter_info.VpdInfo.Tsid);
}
if (!aac_check_reset ||
- (dev->supplement_adapter_info.SupportedOptions2 &
- le32_to_cpu(AAC_OPTION_IGNORE_RESET))) {
+ (dev->supplement_adapter_info.SupportedOptions2 &
+cpu_to_le32(AAC_OPTION_IGNORE_RESET))) {
printk(KERN_INFO "%s%d: Reset Adapter Ignored\n",
dev->name, dev->id);
}
Index: linux-2.6/drivers/scsi/aacraid/commsup.c
===
--- linux-2.6.orig/drivers/scsi/aacraid/commsup.c 2007-11-08
17:09:50.0 +0100
+++ linux-2.6/drivers/scsi/aacraid/commsup.c2007-11-08 17:14:43.0
+0100
@@ -796,13 +796,13 @@
*/
switch (le32_to_cpu(aifcmd->command)) {
case AifCmdDriverNotify:
- switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+ switch (le32_to_cpu(((__le32 *)aifcmd->data)[0])) {
/*
* Morph or Expand complete
*/
case AifDenMorphComplete:
case AifDenVolumeExtendComplete:
- container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+ container = le32_to_cpu(((__le32 *)aifcmd->data)[1]);
if (container >= dev->maximum_num_containers)
break;
@@ -835,25 +835,25 @@
if (container >= dev->maximum_num_containers)
break;
if ((dev->fsa_dev[container].config_waiting_on ==
- le32_to_cpu(*(u32 *)aifcmd->data)) &&
+ le32_to_cpu(*(__le32 *)aifcmd->data)) &&
time_before(jiffies,
dev->fsa_dev[container].config_waiting_stamp + AIF_SNIFF_TIMEOUT))
dev->fsa_dev[container].config_waiting_on = 0;
} else for (container = 0;
container < dev->maximum_num_containers; ++container) {
if ((dev->fsa_dev[container].config_waiting_on ==
- le32_to_cpu(*(u32 *)aifcmd->data)) &&
+ le32_to_cpu(*(__le32 *)aifcmd->data)) &&
time_before(jiffies,
dev->fsa_dev[container].config_waiting_stamp + AIF_SNIFF_TIMEOUT))
dev->fsa_dev[container].config_waiting_on = 0;
}
break;
case AifCmdEventNotify:
- switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+ switch (le32_to_cpu(((__le32 *)aifcmd->data)[0])) {
/*
* Add an Array.
*/
case AifEnAddContainer:
- container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+ container = le32_to_cpu(((__le32 *)aifcmd->data)[1]);
if (container >= dev->maximum_num_containers)
break;
dev->fsa_dev[container].config_needed = ADD;
@@ -866,7 +866,7 @@
* Delete an Array.
[PATCH 4/4] SCSI: bidi support
At the block level bidi request uses req->next_rq pointer for a second
bidi_read request.
At Scsi-midlayer a second scsi_data_buffer structure is used for the
bidi_read part. This bidi scsi_data_buffer is put on
request->next_rq->special. Struct scsi_cmnd is not changed.
- Define scsi_bidi_cmnd() to return true if it is a bidi request and a
second sgtable was allocated.
- Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer
from this command This API is to isolate users from the mechanics of
bidi.
- Define scsi_end_bidi_request() to do what scsi_end_request() does but
for a bidi request. This is necessary because bidi commands are a bit
tricky here. (See comments in body)
- scsi_release_buffers() will also release the bidi_read scsi_data_buffer
- scsi_io_completion() on bidi commands will now call
scsi_end_bidi_request() and return.
- The previous work done in scsi_init_io() is now done in a new
scsi_init_sgtable() (which is 99% identical to old scsi_init_io())
The new scsi_init_io() will call the above twice if needed also for
the bidi_read command. Only at this point is a command bidi.
- In scsi_error.c at scsi_eh_prep/restore_cmnd() make sure bidi-lld is not
confused by a get-sense command that looks like bidi. This is done
by puting NULL at request->next_rq, and restoring.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_error.c |3 +
drivers/scsi/scsi_lib.c | 154 -
include/scsi/scsi_cmnd.h | 23 +++-
include/scsi/scsi_eh.h|1 +
4 files changed, 149 insertions(+), 32 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 9daa983..13b6802 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -618,9 +618,11 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
ses->sdb = scmd->sdb;
+ ses->next_rq = scmd->request->next_rq;
ses->result = scmd->result;
memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+ scmd->request->next_rq = NULL;
if (sense_bytes) {
scmd->sdb.length = min_t(unsigned,
@@ -673,6 +675,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct
scsi_eh_save *ses)
memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
scmd->sc_data_direction = ses->data_direction;
scmd->sdb = ses->sdb;
+ scmd->request->next_rq = ses->next_rq;
scmd->result = ses->result;
}
EXPORT_SYMBOL(scsi_eh_restore_cmnd);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index edfd8d8..1ed6c6b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -64,6 +64,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
};
#undef SP
+static struct kmem_cache *scsi_bidi_sdb_cache;
+
static void scsi_run_queue(struct request_queue *q);
/*
@@ -625,6 +627,28 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
+static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
+{
+ struct request_queue *q = cmd->device->request_queue;
+ struct request *req = cmd->request;
+ unsigned long flags;
+
+ add_disk_randomness(req->rq_disk);
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ if (blk_rq_tagged(req))
+ blk_queue_end_tag(q, req);
+
+ end_that_request_last(req, uptodate);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ /*
+* This will goose the queue request function at the end, so we don't
+* need to worry about launching another command.
+*/
+ scsi_next_command(cmd);
+}
+
/*
* Function:scsi_end_request()
*
@@ -652,7 +676,6 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
{
struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
- unsigned long flags;
/*
* If there are blocks left over at the end, set up the command
@@ -681,19 +704,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
}
}
- add_disk_randomness(req->rq_disk);
-
- spin_lock_irqsave(q->queue_lock, flags);
- if (blk_rq_tagged(req))
- blk_queue_end_tag(q, req);
- end_that_request_last(req, uptodate);
- spin_unlock_irqrestore(q->queue_lock, flags);
-
- /*
-* This will goose the queue request function at the end, so we don't
-* need to worry about launching another command.
-*/
- scsi_next_command(cmd);
+ scsi_finalize_request(cmd, uptodate);
return NULL;
}
@@ -892,10 +903,47 @@ void scsi_release_buffers(struct scsi_cmnd *cmd)
scsi_fre
[PATCH 3/4] scsi_data_buffer
In preparation for bidi we abstract all IO members of scsi_cmnd,
that will need to duplicate, into a substructure.
- Group all IO members of scsi_cmnd into a scsi_data_buffer
structure.
- Adjust accessors to new members.
- scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
scsi_cmnd. And work on it.
- Adjust scsi_init_io() and scsi_release_buffers() for above
change.
- Fix other parts of scsi_lib/scsi.c to members migration. Use
accessors where appropriate.
- fix Documentation about scsi_cmnd in scsi_host.h
- scsi_error.c
* Changed needed members of struct scsi_eh_save.
* Careful considerations in scsi_eh_prep/restore_cmnd.
- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block
size so code needs to change once we move to scsi_data_buff
implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
- tgt: convert libsrp to use scsi_data_buffer
- isd200: This driver still bangs on scsi_cmnd IO members,
so need changing
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
drivers/scsi/libsrp.c|4 +-
drivers/scsi/scsi.c |2 +-
drivers/scsi/scsi_error.c| 28 +-
drivers/scsi/scsi_lib.c | 79 --
drivers/scsi/sd.c|4 +-
drivers/scsi/sr.c| 25 +++--
drivers/usb/storage/isd200.c |8 ++--
include/scsi/scsi_cmnd.h | 39 +---
include/scsi/scsi_eh.h |8 ++---
include/scsi/scsi_host.h |4 +-
10 files changed, 92 insertions(+), 109 deletions(-)
diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 5cff020..8a8562a 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd
*cmd, void *info,
sc->SCp.ptr = info;
memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
- sc->request_bufflen = len;
- sc->request_buffer = (void *) (unsigned long) addr;
+ sc->sdb.length = len;
+ sc->sdb.sglist = (void *) (unsigned long) addr;
sc->tag = tag;
err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
cmd->tag);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1929488..73d2216 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -698,7 +698,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
"Notifying upper driver of completion "
"(result %x)\n", cmd->result));
- good_bytes = cmd->request_bufflen;
+ good_bytes = scsi_bufflen(cmd);
if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
drv = scsi_cmd_to_driver(cmd);
if (drv->done)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ebaca4c..9daa983 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
ses->cmd_len = scmd->cmd_len;
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
- ses->bufflen = scmd->request_bufflen;
- ses->buffer = scmd->request_buffer;
- ses->use_sg = scmd->use_sg;
- ses->resid = scmd->resid;
+ ses->sdb = scmd->sdb;
ses->result = scmd->result;
+ memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+
if (sense_bytes) {
- scmd->request_bufflen = min_t(unsigned,
+ scmd->sdb.length = min_t(unsigned,
sizeof(scmd->sense_buffer), sense_bytes);
sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
- scmd->request_bufflen);
- scmd->request_buffer = &ses->sense_sgl;
+ scmd->sdb.length);
+ scmd->sdb.sglist = &ses->sense_sgl;
scmd->sc_data_direction = DMA_FROM_DEVICE;
- scmd->use_sg = 1;
+ scmd->sdb.sg_count = 1;
memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
scmd->cmnd[0] = REQUEST_SENSE;
- scmd->cmnd[4] = scmd->request_bufflen;
+ scmd->cmnd[4] = scmd->sdb.length;
scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
} else {
- scmd->request_buffer = NULL;
- scmd->request_bufflen = 0;
scmd->sc_data_direction = DMA_NONE;
- scmd->use_sg = 0;
if (cmnd) {
memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
memcpy(scmd->cmnd, cmnd, cmnd_size);
@@ -676,10 +672,7 @@ void scsi_eh_restore_cmnd(struct scsi_cm
[PATCH 2/4] tgt: Use scsi_init_io instead of scsi_alloc_sgtable
- If we export scsi_init_io()/scsi_release_buffers() instead of
scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
much more insulated from scsi_lib changes. As a bonus it will
also gain bidi capability when it comes.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Acked-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_lib.c | 21 ++---
drivers/scsi/scsi_tgt_lib.c | 34 +-
include/scsi/scsi_cmnd.h|4 ++--
3 files changed, 17 insertions(+), 42 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0e81e4c..6d8ea69 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -737,7 +737,8 @@ static inline unsigned int scsi_sgtable_index(unsigned
short nents)
return index;
}
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
+ gfp_t gfp_mask)
{
struct scsi_host_sg_pool *sgp;
struct scatterlist *sgl, *prev, *ret;
@@ -823,9 +824,7 @@ enomem:
return NULL;
}
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_cmnd *cmd)
{
struct scatterlist *sgl = cmd->request_buffer;
struct scsi_host_sg_pool *sgp;
@@ -871,8 +870,6 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
mempool_free(sgl, sgp->pool);
}
-EXPORT_SYMBOL(scsi_free_sgtable);
-
/*
* Function:scsi_release_buffers()
*
@@ -890,7 +887,7 @@ EXPORT_SYMBOL(scsi_free_sgtable);
* the scatter-gather table, and potentially any bounce
* buffers.
*/
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
+void scsi_release_buffers(struct scsi_cmnd *cmd)
{
if (cmd->use_sg)
scsi_free_sgtable(cmd);
@@ -902,6 +899,7 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
cmd->request_buffer = NULL;
cmd->request_bufflen = 0;
}
+EXPORT_SYMBOL(scsi_release_buffers);
/*
* Function:scsi_io_completion()
@@ -1104,7 +1102,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* BLKPREP_DEFER if the failure is retryable
* BLKPREP_KILL if the failure is fatal
*/
-static int scsi_init_io(struct scsi_cmnd *cmd)
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
{
struct request *req = cmd->request;
intcount;
@@ -1119,7 +1117,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
/*
* If sg table allocation fails, requeue request later.
*/
- cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
+ cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
if (unlikely(!cmd->request_buffer)) {
scsi_unprep_request(req);
return BLKPREP_DEFER;
@@ -1148,6 +1146,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
return BLKPREP_KILL;
}
+EXPORT_SYMBOL(scsi_init_io);
static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
struct request *req)
@@ -1193,7 +1192,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev,
struct request *req)
BUG_ON(!req->nr_phys_segments);
- ret = scsi_init_io(cmd);
+ ret = scsi_init_io(cmd, GFP_ATOMIC);
if (unlikely(ret))
return ret;
} else {
@@ -1244,7 +1243,7 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct
request *req)
if (unlikely(!cmd))
return BLKPREP_DEFER;
- return scsi_init_io(cmd);
+ return scsi_init_io(cmd, GFP_ATOMIC);
}
EXPORT_SYMBOL(scsi_setup_fs_cmnd);
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index deea3cd..5fd5fca 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -331,8 +331,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
- if (scsi_sglist(cmd))
- scsi_free_sgtable(cmd);
+ scsi_release_buffers(cmd);
queue_work(scsi_tgtd, &tcmd->work);
}
@@ -353,31 +352,6 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd
*cmd)
return 0;
}
-static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
-{
- struct request *rq = cmd->request;
- int count;
-
- cmd->use_sg = rq->nr_phys_segments;
- cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
- if (!cmd->request_buffer)
- return -ENOMEM;
-
- cmd->request_bufflen = rq->data_len;
-
- dprintk("cmd %p cnt %d %lu\n", cmd, scsi_sg_count(cmd),
- rq_data_dir(rq));
- count = blk_rq_map_sg(rq->q, rq, scsi_sglist(cmd));
- if (likely(count <= scsi_sg_count
[PATCH 1/4] sr/sd: Remove dead code
if (rq_data_dir() == WRITE) else if() else chain had an extra
"else" since the if() is on a value of 1 bit.
Also with a bidi request, rq_data_dir() == WRITE
and blk_bidi_rq() == true.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/sd.c |5 +
drivers/scsi/sr.c |5 +
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 18343a6..f1ec2bd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -445,12 +445,9 @@ static int sd_prep_fn(struct request_queue *q, struct
request *rq)
}
SCpnt->cmnd[0] = WRITE_6;
SCpnt->sc_data_direction = DMA_TO_DEVICE;
- } else if (rq_data_dir(rq) == READ) {
+ } else {
SCpnt->cmnd[0] = READ_6;
SCpnt->sc_data_direction = DMA_FROM_DEVICE;
- } else {
- scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n",
rq->cmd_flags);
- goto out;
}
SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7702681..a72ae85 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -365,12 +365,9 @@ static int sr_prep_fn(struct request_queue *q, struct
request *rq)
SCpnt->cmnd[0] = WRITE_10;
SCpnt->sc_data_direction = DMA_TO_DEVICE;
cd->cdi.media_written = 1;
- } else if (rq_data_dir(rq) == READ) {
+ } else {
SCpnt->cmnd[0] = READ_10;
SCpnt->sc_data_direction = DMA_FROM_DEVICE;
- } else {
- blk_dump_rq_flags(rq, "Unknown sr command");
- goto out;
}
{
--
1.5.3.1
-
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
Re: [0/4 ver2] Last 3 patches for bidi support
On Tue, Nov 06 2007 at 20:04 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote: > [1] > I propose a small change to scsi_tgt_lib.c that will make > tgt completely neutral to the scsi_data_buffer patch. And will > make it all that more ready for bidi, too. TOMO is this OK? > > (Can you do without the GFP_KERNEL allocation flag? It could > make the code a bit more simple) > > [2] > scsi_data_buffer patch. > As requested by TOMO the all patch is squashed into one, 600 > and some lines. TOMO if it makes you happy, OK, here it is. > > There is one hunk from libsrp.c that I really hate. and from what > I understand of the code, only the "request_bufflen =" is really used, > All users of "request_buffer = addr" pass NULL, as far as I can see. > If you would like to make me happy, and it is at all possible, please > clean it. > > [3] > Once scsi_data_buffer is in, then scsi bidi support can be applied. > (Block bidi was already merged 3 kernels ago). It makes no changes > to scsi_cmnd and only adds some, not-at-all-dangerous, code to scsi_lib. > So I don't see any reason to wait. please all review. > > If ACKed by all parties and applied for inclusion for 2.6.25, > then they will have a long time to sit in MM and collect > compilation breakage reports from ARCHs we never compiled. > > I wish these make everybody happy this time > > Boaz Harrosh > Version 2, fixed as of Tomo's comments. Thanks Tomo. [0] Remove dead code from sd.c & sr.c. It was conceived by programmers in the passed that rq_data_dir() could perhaps be expanded to 2 bits and return READ/WRITE/BIDI. But we have decided long ago that a bidi request is when blk_bidi_rq() == true, and in that case rq_data_dir() == WRITE. So now we can be sure of what the compiler knew for a long time. (The future is already here) [1] [2] [3] See above I must insist again they should have time to sit in -mm. For posible compilation breakage of other ARCHs. Boaz - 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
Re: LTO-3 read performance issues
Kai Makisara wrote: I am using the default st module options and not doing anything other than using 'mt setblk 0'. Is there anything I can do to get a decent, sustained read rate from these tapes? If your system can process the data fast enough and the tape blocks are of the same size, you can try using fixed block mode and buffered transfers. This allows the driver to use larger SCSI reads. You have to load the st module with parameter buffer_kbs=xxx, where xxx is, for instance, 1024 (1 MB buffer). You also have to disable direct i/o with the module parameter try_direct_io=0 (otherwise buffered transfers are disabled). Thanks, I tried that, but it makes no difference. I also tried increasing the buffer_kbs size by doubling it a few times, but that made no difference either I have now found out that the tapes were written on an SGI system - using a fixed block device and the default options to tar. However, I have to either use 'mt setblk 0' or 'mt setblk 16384' to read anything from these tapes. Just to check that nothing is wrong with my system, I can read an LTO-3 tape previously written on my system, using the default options to tar and the st module - and I get over 40Mb/s on reads, compared with about 400Kb/s I'm getting with these problematic tapes ... Is there anything else I can do to read these tapes at a decent speed? Thanks James Pearson - 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
Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable
On Thu, 08 Nov 2007 16:01:53 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> On Thu, Nov 08 2007 at 15:04 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> > On Thu, 08 Nov 2007 10:32:56 +0200
> > Benny Halevy <[EMAIL PROTECTED]> wrote:
> >
> >> On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> >>> On Tue, 06 Nov 2007 20:16:19 +0200
> >>> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> >>>
> - If we export scsi_init_io()/scsi_release_buffers() instead of
> scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
> much more insulated from scsi_lib changes. As a bonus it will
> also gain bidi capability when it comes.
>
> Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
> >>> Looks good for me except for this:
> >>>
> >>> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
> >>> ERROR: use tabs not spaces
> >>> #101: FILE: drivers/scsi/scsi_lib.c:741:
> >>> + gfp_t gfp_mask)$
> >> Come on Tomo, tabs should be used for nesting, not for decoration.
> >> This way no matter what's your tab expansion setup is the
> >> code will look correct and will make sense. The number of space
> >
> > I've never heard about that rule. I use tabs and minimum spaces for
> > decoration.
> >
> > But it's just about the style. The patch is fine by me if you like to
> > use only spaces there.
> >
> > Thanks,
> Thanks Tomo, I'm resending with the way you like it. You are
> the maintainer and you should be comfortable with the code.
Actually, checkpatch complains about scsi_lib part. You don't need my
ACK on that part.
> I do need your Signed-off-by on this. Since you are the maintainer.
> Do you want that we push this through James, or through your tree?
Acked-by instead of Signed-off-by, I guess.
ACK on target mode portion. You can directly send it to James.
Oh, one more trivial stuff. Can you use 'tgt: hoge' subject? We
usually use that style for target mode though it doesn't matter much.
Thanks,
-
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
Re: [PATCH 2/3] scsi_data_buffer
On Thu, Nov 08 2007 at 15:54 +0200, Jens Axboe <[EMAIL PROTECTED]> wrote:
> On Thu, Nov 08 2007, Boaz Harrosh wrote:
>> James, Jens please note the question below
>> It is something that bothers me about sr.c
>>
>> On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>>> In preparation for bidi we abstract all IO members of scsi_cmnd,
>>> that will need to duplicate, into a substructure.
>>>
>>
>>> - sd.c and sr.c
>>> * sd and sr would adjust IO size to align on device's block
>>> size so code needs to change once we move to scsi_data_buff
>>> implementation.
>>> * Convert code to use scsi_for_each_sg
>>> * Use data accessors where appropriate.
>>> * Remove dead code (req_data_dir() != READ && != WRITE)
>>>
>>
>>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>>> index 7702681..6d3bf41 100644
>>> --- a/drivers/scsi/sr.c
>>> +++ b/drivers/scsi/sr.c
>>> @@ -226,7 +226,7 @@ out:
>>> static int sr_done(struct scsi_cmnd *SCpnt)
>>> {
>>> int result = SCpnt->result;
>>> - int this_count = SCpnt->request_bufflen;
>>> + int this_count = scsi_bufflen(SCpnt);
>>> int good_bytes = (result == 0 ? this_count : 0);
>>> int block_sectors = 0;
>>> long error_sector;
>>> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct
>>> request *rq)
>>> } else if (rq_data_dir(rq) == READ) {
>>> SCpnt->cmnd[0] = READ_10;
>>> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>>> - } else {
>>> - blk_dump_rq_flags(rq, "Unknown sr command");
>>> - goto out;
>>> }
>>>
>>> {
>>> - struct scatterlist *sg = SCpnt->request_buffer;
>>> - int i, size = 0;
>>> - for (i = 0; i < SCpnt->use_sg; i++)
>>> - size += sg[i].length;
>>> + struct scatterlist *sg;
>>> + int i, size = 0, sg_count = scsi_sg_count(SCpnt);
>>> +
>>> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
>>> + size += sg->length;
>>>
>>> - if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
>>> + if (size != scsi_bufflen(SCpnt)) {
>>> scmd_printk(KERN_ERR, SCpnt,
>>> "mismatch count %d, bytes %d\n",
>>> - size, SCpnt->request_bufflen);
>>> - if (SCpnt->request_bufflen > size)
>>> - SCpnt->request_bufflen = size;
>>> + size, scsi_bufflen(SCpnt));
>>> + if (scsi_bufflen(SCpnt) > size)
>>> + SCpnt->sdb.length = size;
>>> }
>>> }
>>>
>>> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct
>>> request *rq)
>>> * request doesn't start on hw block boundary, add scatter pads
>>> */
>>> if (((unsigned int)rq->sector % (s_size >> 9)) ||
>>> - (SCpnt->request_bufflen % s_size)) {
>>> + (scsi_bufflen(SCpnt) % s_size)) {
>>> scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
>>> goto out;
>>> }
>> Here we check I/O is "large-block" aligned. Both start and size
>>
>>>
>>> - this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
>>> + this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>>>
>> Number of "large-blocks"
>>
>>>
>>> SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
>>> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct
>>> request *rq)
>>>
>>> if (this_count > 0x) {
>>> this_count = 0x;
>>> - SCpnt->request_bufflen = this_count * s_size;
>>> + SCpnt->sdb.length = this_count * s_size;
>>> }
>>>
>> Here is my problem:
>> In the case that the transfer is bigger than 0x * s_size
>> (512/1024/2048) we modify ->request_bufflen. Now this has two bad
>> effects, the way I understand it, please fix me in my
>> misunderstanding.
>>
>> 1. Later in sr_done doing return good_bytes=cmd->request_bufflen will
>> only complete the cut-out bytes. Meaning possible BIO leak, since the
>> original request_bufflen was lost. (not all bytes are completed)
>>
>>
>> 2. What mechanics will re-send, or even knows, that not the complete
>> request was transfered? The way I understand it, a cmnd->resid must be
>> set, in this case. Now the normal cmnd->resid is not enough because
>> it will be written by drivers, sr needs to stash a resid somewhere and
>> add it to cmnd->resid in sr_done(). But ...
>>
>
> It's not lost, the request will be requeued in scsi_end_request(). The
> original state is in the request structure, and end_that_request_chunk()
> will return not-done when you complete this first part.
>
OK, I see it now, thanks.
Should we adjust the q->max_hw_sectors anyway so elevator can divide the
work load more evenly? The way it is now, it's possible that we always do
small leftovers at the end.
Boaz
-
To unsubscribe from
Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable
On Thu, Nov 08 2007 at 15:04 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> On Thu, 08 Nov 2007 10:32:56 +0200
> Benny Halevy <[EMAIL PROTECTED]> wrote:
>
>> On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
>>> On Tue, 06 Nov 2007 20:16:19 +0200
>>> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>>>
- If we export scsi_init_io()/scsi_release_buffers() instead of
scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
much more insulated from scsi_lib changes. As a bonus it will
also gain bidi capability when it comes.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
>>> Looks good for me except for this:
>>>
>>> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
>>> ERROR: use tabs not spaces
>>> #101: FILE: drivers/scsi/scsi_lib.c:741:
>>> + gfp_t gfp_mask)$
>> Come on Tomo, tabs should be used for nesting, not for decoration.
>> This way no matter what's your tab expansion setup is the
>> code will look correct and will make sense. The number of space
>
> I've never heard about that rule. I use tabs and minimum spaces for
> decoration.
>
> But it's just about the style. The patch is fine by me if you like to
> use only spaces there.
>
> Thanks,
Thanks Tomo, I'm resending with the way you like it. You are
the maintainer and you should be comfortable with the code.
I do need your Signed-off-by on this. Since you are the maintainer.
Do you want that we push this through James, or through your tree?
Thanks again, will send revision soon.
Boaz
-
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
Re: [PATCH 2/3] scsi_data_buffer
On Thu, Nov 08 2007, Boaz Harrosh wrote:
> James, Jens please note the question below
> It is something that bothers me about sr.c
>
> On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> > In preparation for bidi we abstract all IO members of scsi_cmnd,
> > that will need to duplicate, into a substructure.
> >
>
> >
> > - sd.c and sr.c
> > * sd and sr would adjust IO size to align on device's block
> > size so code needs to change once we move to scsi_data_buff
> > implementation.
> > * Convert code to use scsi_for_each_sg
> > * Use data accessors where appropriate.
> > * Remove dead code (req_data_dir() != READ && != WRITE)
> >
>
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 7702681..6d3bf41 100644
> > --- a/drivers/scsi/sr.c
> > +++ b/drivers/scsi/sr.c
> > @@ -226,7 +226,7 @@ out:
> > static int sr_done(struct scsi_cmnd *SCpnt)
> > {
> > int result = SCpnt->result;
> > - int this_count = SCpnt->request_bufflen;
> > + int this_count = scsi_bufflen(SCpnt);
> > int good_bytes = (result == 0 ? this_count : 0);
> > int block_sectors = 0;
> > long error_sector;
> > @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct
> > request *rq)
> > } else if (rq_data_dir(rq) == READ) {
> > SCpnt->cmnd[0] = READ_10;
> > SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> > - } else {
> > - blk_dump_rq_flags(rq, "Unknown sr command");
> > - goto out;
> > }
> >
> > {
> > - struct scatterlist *sg = SCpnt->request_buffer;
> > - int i, size = 0;
> > - for (i = 0; i < SCpnt->use_sg; i++)
> > - size += sg[i].length;
> > + struct scatterlist *sg;
> > + int i, size = 0, sg_count = scsi_sg_count(SCpnt);
> > +
> > + scsi_for_each_sg (SCpnt, sg, sg_count, i)
> > + size += sg->length;
> >
> > - if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
> > + if (size != scsi_bufflen(SCpnt)) {
> > scmd_printk(KERN_ERR, SCpnt,
> > "mismatch count %d, bytes %d\n",
> > - size, SCpnt->request_bufflen);
> > - if (SCpnt->request_bufflen > size)
> > - SCpnt->request_bufflen = size;
> > + size, scsi_bufflen(SCpnt));
> > + if (scsi_bufflen(SCpnt) > size)
> > + SCpnt->sdb.length = size;
> > }
> > }
> >
> > @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct
> > request *rq)
> > * request doesn't start on hw block boundary, add scatter pads
> > */
> > if (((unsigned int)rq->sector % (s_size >> 9)) ||
> > - (SCpnt->request_bufflen % s_size)) {
> > + (scsi_bufflen(SCpnt) % s_size)) {
> > scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
> > goto out;
> > }
> Here we check I/O is "large-block" aligned. Both start and size
>
> >
> > - this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
> > + this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
> >
> Number of "large-blocks"
>
> >
> > SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
> > @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct
> > request *rq)
> >
> > if (this_count > 0x) {
> > this_count = 0x;
> > - SCpnt->request_bufflen = this_count * s_size;
> > + SCpnt->sdb.length = this_count * s_size;
> > }
> >
> Here is my problem:
> In the case that the transfer is bigger than 0x * s_size
> (512/1024/2048) we modify ->request_bufflen. Now this has two bad
> effects, the way I understand it, please fix me in my
> misunderstanding.
>
> 1. Later in sr_done doing return good_bytes=cmd->request_bufflen will
> only complete the cut-out bytes. Meaning possible BIO leak, since the
> original request_bufflen was lost. (not all bytes are completed)
>
>
> 2. What mechanics will re-send, or even knows, that not the complete
> request was transfered? The way I understand it, a cmnd->resid must be
> set, in this case. Now the normal cmnd->resid is not enough because
> it will be written by drivers, sr needs to stash a resid somewhere and
> add it to cmnd->resid in sr_done(). But ...
>
It's not lost, the request will be requeued in scsi_end_request(). The
original state is in the request structure, and end_that_request_chunk()
will return not-done when you complete this first part.
--
Jens Axboe
-
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
Re: [PATCH 2/3] scsi_data_buffer
On Thu, Nov 08 2007 at 15:03 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> On Thu, 08 Nov 2007 11:24:36 +0200
> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 18343a6..28cf6fe 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct
request *rq)
} else if (rq_data_dir(rq) == READ) {
SCpnt->cmnd[0] = READ_6;
SCpnt->sc_data_direction = DMA_FROM_DEVICE;
- } else {
- scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n",
rq->cmd_flags);
- goto out;
>>> This should go to the bidi patch?
>>>
}
>> This is just a dead code cleanup. It is got nothing to do with bidi or
>> scsi_data_buffer
>> for that matter. It could be in it's own patch, but surly it will not go
>> into the bidi
>> patch. I will submit a new patch just for that code. Independent of these.
>> (I was hoping to save the extra effort)
>
> Hm, is it dead code? I think it's kinda BUG_ON, that is, we should not
> hit that code. sd only accetps READ and WRITE requests. It prevents
> funcy requests like BIDI from accidentally comming.
It is dead code. The rq_data_dir(rq) does a (->flags & 0x1) inline
the compiler will remove the extra code.
Also with bidi rq_data_dir(rq) is decided to return WRITE, a bidi
request is blk_bidi_rq(rq).
(I have separated this to a patch of it's own.)
Boaz
-
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
Re: [PATCH 2/3] scsi_data_buffer
James, Jens please note the question below
It is something that bothers me about sr.c
On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> In preparation for bidi we abstract all IO members of scsi_cmnd,
> that will need to duplicate, into a substructure.
>
>
> - sd.c and sr.c
> * sd and sr would adjust IO size to align on device's block
> size so code needs to change once we move to scsi_data_buff
> implementation.
> * Convert code to use scsi_for_each_sg
> * Use data accessors where appropriate.
> * Remove dead code (req_data_dir() != READ && != WRITE)
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 7702681..6d3bf41 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -226,7 +226,7 @@ out:
> static int sr_done(struct scsi_cmnd *SCpnt)
> {
> int result = SCpnt->result;
> - int this_count = SCpnt->request_bufflen;
> + int this_count = scsi_bufflen(SCpnt);
> int good_bytes = (result == 0 ? this_count : 0);
> int block_sectors = 0;
> long error_sector;
> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct
> request *rq)
> } else if (rq_data_dir(rq) == READ) {
> SCpnt->cmnd[0] = READ_10;
> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> - } else {
> - blk_dump_rq_flags(rq, "Unknown sr command");
> - goto out;
> }
>
> {
> - struct scatterlist *sg = SCpnt->request_buffer;
> - int i, size = 0;
> - for (i = 0; i < SCpnt->use_sg; i++)
> - size += sg[i].length;
> + struct scatterlist *sg;
> + int i, size = 0, sg_count = scsi_sg_count(SCpnt);
> +
> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
> + size += sg->length;
>
> - if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
> + if (size != scsi_bufflen(SCpnt)) {
> scmd_printk(KERN_ERR, SCpnt,
> "mismatch count %d, bytes %d\n",
> - size, SCpnt->request_bufflen);
> - if (SCpnt->request_bufflen > size)
> - SCpnt->request_bufflen = size;
> + size, scsi_bufflen(SCpnt));
> + if (scsi_bufflen(SCpnt) > size)
> + SCpnt->sdb.length = size;
> }
> }
>
> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct
> request *rq)
>* request doesn't start on hw block boundary, add scatter pads
>*/
> if (((unsigned int)rq->sector % (s_size >> 9)) ||
> - (SCpnt->request_bufflen % s_size)) {
> + (scsi_bufflen(SCpnt) % s_size)) {
> scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
> goto out;
> }
Here we check I/O is "large-block" aligned. Both start and size
>
> - this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
> + this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>
Number of "large-blocks"
>
> SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct
> request *rq)
>
> if (this_count > 0x) {
> this_count = 0x;
> - SCpnt->request_bufflen = this_count * s_size;
> + SCpnt->sdb.length = this_count * s_size;
> }
>
Here is my problem:
In the case that the transfer is bigger than 0x * s_size (512/1024/2048)
we modify ->request_bufflen. Now this has two bad effects, the way I understand
it,
please fix me in my misunderstanding.
1. Later in sr_done doing return good_bytes=cmd->request_bufflen will only
complete
the cut-out bytes. Meaning possible BIO leak, since the original
request_bufflen
was lost. (not all bytes are completed)
2. What mechanics will re-send, or even knows, that not the complete request
was
transfered? The way I understand it, a cmnd->resid must be set, in this case.
Now the normal cmnd->resid is not enough because it will be written by
drivers, sr needs to stash a resid somewhere and add it to cmnd->resid in
sr_done(). But ...
I have a better solution for this. At attachment time. sr will modify the
request_queue's max_hw_sectors to not max over 0x * s_size, this way
the block layer will split it's I/O, and no extra resid handling is needed.
If the later is accepted than where this blk_set_max_hw_sectors() be?
on the first request, like block-size. Or at sr_open()/sr_block_open()
Please comment and I'll scribble a patch.
> SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 178e8c2..2d9a32b 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
Thanks
Boaz
-
To unsubsc
Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable
On Thu, 08 Nov 2007 10:32:56 +0200
Benny Halevy <[EMAIL PROTECTED]> wrote:
> On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> > On Tue, 06 Nov 2007 20:16:19 +0200
> > Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> >
> >> - If we export scsi_init_io()/scsi_release_buffers() instead of
> >> scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
> >> much more insulated from scsi_lib changes. As a bonus it will
> >> also gain bidi capability when it comes.
> >>
> >> Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
> >
> > Looks good for me except for this:
> >
> > ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
> > ERROR: use tabs not spaces
> > #101: FILE: drivers/scsi/scsi_lib.c:741:
> > + gfp_t gfp_mask)$
>
> Come on Tomo, tabs should be used for nesting, not for decoration.
> This way no matter what's your tab expansion setup is the
> code will look correct and will make sense. The number of space
I've never heard about that rule. I use tabs and minimum spaces for
decoration.
But it's just about the style. The patch is fine by me if you like to
use only spaces there.
Thanks,
-
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
Re: [PATCH 2/3] scsi_data_buffer
On Thu, 08 Nov 2007 11:24:36 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >> index 18343a6..28cf6fe 100644
> >> --- a/drivers/scsi/sd.c
> >> +++ b/drivers/scsi/sd.c
> >> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct
> >> request *rq)
> >>} else if (rq_data_dir(rq) == READ) {
> >>SCpnt->cmnd[0] = READ_6;
> >>SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> >> - } else {
> >> - scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n",
> >> rq->cmd_flags);
> >> - goto out;
> >
> > This should go to the bidi patch?
> >
> >>}
> >>
> This is just a dead code cleanup. It is got nothing to do with bidi or
> scsi_data_buffer
> for that matter. It could be in it's own patch, but surly it will not go into
> the bidi
> patch. I will submit a new patch just for that code. Independent of these.
> (I was hoping to save the extra effort)
Hm, is it dead code? I think it's kinda BUG_ON, that is, we should not
hit that code. sd only accetps READ and WRITE requests. It prevents
funcy requests like BIDI from accidentally comming.
-
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
Re: [PATCH 2/3] scsi_data_buffer
On Thu, Nov 08 2007 at 5:14 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> On Tue, 06 Nov 2007 20:19:32 +0200
> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>
>
> Hmm, checkpatch.pl complains reasonably:
>
> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28815
> ERROR: use tabs not spaces
> #177: FILE: drivers/scsi/scsi_error.c:629:
> +^I^I scmd->sdb.length);$
>
> ERROR: use tabs not spaces
> #237: FILE: drivers/scsi/scsi_lib.c:741:
> + unsigned short sg_count, gfp_t
> gfp_mask)$
>
> WARNING: no space between function name and open parenthesis '('
> #487: FILE: drivers/scsi/sr.c:377:
> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
>
> ERROR: "foo* bar" should be "foo *bar"
> #563: FILE: include/scsi/scsi_cmnd.h:20:
> + struct scatterlist* sglist;
>
> total: 3 errors, 1 warnings, 482 lines checked
I think that checkpatch is wrong in two accounts.
1. Tabs are used for "Indents" not "align-to-the-right".
All above have 0 indent and are right aligned for
readability.
2. check patch should be fixed that if a macro is followed
by a "{" it means a control block and not a function-call/
macro-expansion, and a space is recommended.
(Like: for, if, while ...)
But I don't mind. I'll change all in a fixing patch.
>
>
>> @@ -821,24 +820,24 @@ enomem:
>>
>> mempool_free(prev, sgp->pool);
>> }
>> -return NULL;
>> +return -1;
>
> I think that -ENOMEM is better. The other functions in scsi_lib.c
> (even static functions) use proper error values.
>
>
will fix, thanks.
>> -/*
>> - * If sg table allocation fails, requeue request later.
>> - */
>> -cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
>> -if (unlikely(!cmd->request_buffer)) {
>> +if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
>
> IIRC, preferable style is:
>
> ret = scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask);
> if (ret) {
>
>
It is more readable I agree, but I did not want to allocate one more
stack variable just for the if(), since I am returning a BLKPREP_DEFER
any way. I am not using ret for anything else.
>> scsi_unprep_request(req);
>> return BLKPREP_DEFER;
>> }
>>
>> req->buffer = NULL;
>> if (blk_pc_request(req))
>> -cmd->request_bufflen = req->data_len;
>> +sdb->length = req->data_len;
>> else
>> -cmd->request_bufflen = req->nr_sectors << 9;
>> +sdb->length = req->nr_sectors << 9;
>>
>> /*
>> * Next, walk the list, and fill in the addresses and sizes of
>> * each segment.
>> */
>> -count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
>> -if (likely(count <= cmd->use_sg)) {
>> -cmd->use_sg = count;
>> +count = blk_rq_map_sg(req->q, req, sdb->sglist);
>> +if (likely(count <= sdb->sg_count)) {
>> +sdb->sg_count = count;
>> return BLKPREP_OK;
>> }
>>
>> printk(KERN_ERR "Incorrect number of segments after building list\n");
>> -printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
>> +printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
>> printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
>> req->current_nr_sectors);
>>
>> @@ -1199,9 +1182,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev,
>> struct request *req)
>> BUG_ON(req->data_len);
>> BUG_ON(req->data);
>>
>> -cmd->request_bufflen = 0;
>> -cmd->request_buffer = NULL;
>> -cmd->use_sg = 0;
>> +memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>> req->buffer = NULL;
>> }
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 18343a6..28cf6fe 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct
>> request *rq)
>> } else if (rq_data_dir(rq) == READ) {
>> SCpnt->cmnd[0] = READ_6;
>> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> -} else {
>> -scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n",
>> rq->cmd_flags);
>> -goto out;
>
> This should go to the bidi patch?
>
>> }
>>
This is just a dead code cleanup. It is got nothing to do with bidi or
scsi_data_buffer
for that matter. It could be in it's own patch, but surly it will not go into
the bidi
patch. I will submit a new patch just for that code. Independent of these.
(I was hoping to save the extra effort)
>> SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
>> @@ -510,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct
>> request *rq)
>> SCpnt->cmnd[4] = (unsigned char) this_count;
>> SCpnt->cmnd[5] = 0;
>> }
>> -SCpnt->request_bufflen = this_count
Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable
On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> On Tue, 06 Nov 2007 20:16:19 +0200
> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>
>> - If we export scsi_init_io()/scsi_release_buffers() instead of
>> scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
>> much more insulated from scsi_lib changes. As a bonus it will
>> also gain bidi capability when it comes.
>>
>> Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
>
> Looks good for me except for this:
>
> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
> ERROR: use tabs not spaces
> #101: FILE: drivers/scsi/scsi_lib.c:741:
> + gfp_t gfp_mask)$
Come on Tomo, tabs should be used for nesting, not for decoration.
This way no matter what's your tab expansion setup is the
code will look correct and will make sense. The number of space
characters in this case is equal to the number of characters in the
line above and with fixed-width fonts the line will be indented
just as you wanted.
The bottom line is that you should indent with as many tabs as your
nesting level, where all statements will begin, and from there on
use space characters.
For example:
{
if (very_long_expression &&
it_needs_to_be_broken_into_several_lines)
return a_very_long_result +
the_remainder_of_it_that_spilled_off +
to_the_next_lines;
return printk("just my %d cents\n",
2);
}
Benny
>
> total: 1 errors, 0 warnings, 144 lines checked
-
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
