Crowd,
Apologies if this is not formatted correctly, but I'm fairly new to this
(submitting patches that is)..
**************
s390 dasd ECKD drivers issues a Perform Subsystem Function / Prepare
for Read SubSystem Data with a length of 16.
However, older hardware and documentation specifies a length of 12
leading to a possible Incorrect Length indication.
This patch activates the SLI CCW flag in order to avoid reporting
the Incorrect Length indication since it is possible that the DASD
control unit may be expecting a length of 12, not 16.
-- Ivan Warren ([email protected])
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 773b3fe..c4e3935 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -810,6 +810,7 @@ static int dasd_eckd_read_features(struct
dasd_device *device)
ccw->cmd_code = DASD_ECKD_CCW_PSF;
ccw->count = sizeof(struct dasd_psf_prssd_data);
ccw->flags |= CCW_FLAG_CC;
+ ccw->flags |= CCW_FLAG_SLI;
ccw->cda = (__u32)(addr_t) prssdp;
/* Read Subsystem Data - feature codes */
**************
--Ivan
Forwarding to s390 kernel list as there have been no replies so far.
(Patch may have whitespace damage because of forward.)
Ivan: You may want to read Documentation/SubmittingPatches in the kernel
source tree.
--------------------------- Forwarded message ---------------------------
Crowd,
Apologies if this is not formatted correctly, but I'm fairly new to this
(submitting patches that is)..
**************
s390 dasd ECKD drivers issues a Perform Subsystem Function / Prepare
for Read SubSystem Data with a length of 16.
However, older hardware and documentation specifies a length of 12
leading to a possible Incorrect Length indication.
This patch activates the SLI CCW flag in order to avoid reporting
the Incorrect Length indication since it is possible that the DASD
control unit may be expecting a length of 12, not 16.
-- Ivan Warren ([email protected])
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c index 773b3fe..c4e3935 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -810,6 +810,7 @@ static int dasd_eckd_read_features(struct dasd_device *device)
ccw->cmd_code = DASD_ECKD_CCW_PSF;
ccw->count = sizeof(struct dasd_psf_prssd_data);
ccw->flags |= CCW_FLAG_CC;
+ ccw->flags |= CCW_FLAG_SLI;
ccw->cda = (__u32)(addr_t) prssdp;
/* Read Subsystem Data - feature codes */
**************
--Ivan
On Thu, 2008-08-14 at 01:07 +0200, Frans Pop wrote:
> s390 dasd ECKD drivers issues a Perform Subsystem Function / Prepare
> for Read SubSystem Data with a length of 16.
> However, older hardware and documentation specifies a length of 12
> leading to a possible Incorrect Length indication.
> This patch activates the SLI CCW flag in order to avoid reporting
> the Incorrect Length indication since it is possible that the DASD
> control unit may be expecting a length of 12, not 16.
> -- Ivan Warren ([email protected])
>
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c index 773b3fe..c4e3935 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -810,6 +810,7 @@ static int dasd_eckd_read_features(struct dasd_device *device)
> ccw->cmd_code = DASD_ECKD_CCW_PSF;
> ccw->count = sizeof(struct dasd_psf_prssd_data);
> ccw->flags |= CCW_FLAG_CC;
> + ccw->flags |= CCW_FLAG_SLI;
> ccw->cda = (__u32)(addr_t) prssdp;
>
> /* Read Subsystem Data - feature codes */
That makes sense, can I get a signed-off-by for the patch please?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
s390 Dasd ECKD Driver issues a Perform Subsystem Function/Prepare
for Read Subsystem Data with a length of 16.
However, some hardware (namely 3990/9390 and some version of ESS)
only take 12 bytes and therefore, an Incorrect Length indication is
returned, breaking CCW Command Chaining and leads to a failure to
initialize the DASD. This patch sets the SLI CCW bit to prevent an
incorrect length indication to be reported when the control unit
expects a length of 12 instead of 16.
Signed-off-by: Ivan Warren <[email protected]>
---
drivers/s390/block/dasd_eckd.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 773b3fe..d644715 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -809,7 +809,7 @@ static int dasd_eckd_read_features(struct dasd_device *device)
ccw = cqr->cpaddr;
ccw->cmd_code = DASD_ECKD_CCW_PSF;
ccw->count = sizeof(struct dasd_psf_prssd_data);
- ccw->flags |= CCW_FLAG_CC;
+ ccw->flags |= CCW_FLAG_CC|CCW_FLAG_SLI;
ccw->cda = (__u32)(addr_t) prssdp;
/* Read Subsystem Data - feature codes */
--
1.5.6.3
Ivan Warren wrote:
> s390 Dasd ECKD Driver issues a Perform Subsystem Function/Prepare
> for Read Subsystem Data with a length of 16.
> However, some hardware (namely 3990/9390 and some version of ESS)
> only take 12 bytes and therefore, an Incorrect Length indication is
> returned, breaking CCW Command Chaining and leads to a failure to
> initialize the DASD. This patch sets the SLI CCW bit to prevent an
> incorrect length indication to be reported when the control unit
> expects a length of 12 instead of 16.
Hi Ivan,
while I agree that your patch is technically correct, I think I'd
rather like to fix the problematic data length instead of making
the storage server ignore it. There is no need for the
dasd_psf_prssd_data structure to be 16 bytes long as it is only used
for suborders that require 12 bytes. The current layout of that
structure was probably just a mistake. If we ever go to use a
suborder that requires more data, we should either define a further
structure specific to that purpose or find some other way to make
sure that each suborder is called with the appropriate data length.
The following patch works fine on current hardware. Can you please
verify that it indeed fixes your problem as well?
Signed-off-by: Stefan Weinhuber <[email protected]>
---
drivers/s390/block/dasd_eckd.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6.26/drivers/s390/block/dasd_eckd.h
===================================================================
--- linux-2.6.26.orig/drivers/s390/block/dasd_eckd.h
+++ linux-2.6.26/drivers/s390/block/dasd_eckd.h
@@ -379,7 +379,7 @@ struct dasd_psf_prssd_data {
unsigned char flags;
unsigned char reserved[4];
unsigned char suborder;
- unsigned char varies[9];
+ unsigned char varies[5];
} __attribute__ ((packed));
/*
Stefan Weinhuber wrote:
> Hi Ivan,
>
> while I agree that your patch is technically correct, I think I'd
> rather like to fix the problematic data length instead of making
> the storage server ignore it. There is no need for the
> dasd_psf_prssd_data structure to be 16 bytes long as it is only used
> for suborders that require 12 bytes. The current layout of that
> structure was probably just a mistake. If we ever go to use a
> suborder that requires more data, we should either define a further
> structure specific to that purpose or find some other way to make
> sure that each suborder is called with the appropriate data length.
>
> The following patch works fine on current hardware. Can you please
> verify that it indeed fixes your problem as well?
>
> Signed-off-by: Stefan Weinhuber <[email protected]>
> ---
> drivers/s390/block/dasd_eckd.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux-2.6.26/drivers/s390/block/dasd_eckd.h
> ===================================================================
> --- linux-2.6.26.orig/drivers/s390/block/dasd_eckd.h
> +++ linux-2.6.26/drivers/s390/block/dasd_eckd.h
> @@ -379,7 +379,7 @@ struct dasd_psf_prssd_data {
> unsigned char flags;
> unsigned char reserved[4];
> unsigned char suborder;
> - unsigned char varies[9];
> + unsigned char varies[5];
> } __attribute__ ((packed));
>
> /*
>
>
Stefan,
Sorry for taking such a long time to respond..
I like the patch. You are right, it's probably better to set the right
length then hide it ! My initial rationale to SLI instead of altering
the length was that I couldn't know if there was any obscure reason for
setting the length to 16 instead of 12 (I thought it could have been
related to DS8K PAV/HyperPAV support)..
I also tried it on my rig, and it does seem to take care of the problem !
So I guess :
Reviewed-by: Ivan Warren <[email protected]>
Tested-by: Ivan Warren <[email protected]>
Thanks again !
--Ivan
On Monday 18 August 2008, Ivan Warren wrote:
> > The following patch works fine on current hardware. Can you please
> > verify that it indeed fixes your problem as well?
> >
> > Signed-off-by: Stefan Weinhuber <[email protected]>
[...]
> Sorry for taking such a long time to respond..
>
> I like the patch. You are right, it's probably better to set the right
> length then hide it ! My initial rationale to SLI instead of altering
> the length was that I couldn't know if there was any obscure reason for
> setting the length to 16 instead of 12 (I thought it could have been
> related to DS8K PAV/HyperPAV support)..
>
> I also tried it on my rig, and it does seem to take care of the problem
>
> Reviewed-by: Ivan Warren <[email protected]>
> Tested-by: Ivan Warren <[email protected]>
Could this patch please also be pushed for the next stable update (2.6.26
and maybe even 2.6.25) by adding:
CC: stable <[email protected]>
?
TIA,
FJP
On Mon, Aug 18, 2008 at 07:56:49PM +0200, Frans Pop wrote:
> On Monday 18 August 2008, Ivan Warren wrote:
> > > The following patch works fine on current hardware. Can you please
> > > verify that it indeed fixes your problem as well?
> > >
> > > Signed-off-by: Stefan Weinhuber <[email protected]>
>
> [...]
>
> > Sorry for taking such a long time to respond..
> >
> > I like the patch. You are right, it's probably better to set the right
> > length then hide it ! My initial rationale to SLI instead of altering
> > the length was that I couldn't know if there was any obscure reason for
> > setting the length to 16 instead of 12 (I thought it could have been
> > related to DS8K PAV/HyperPAV support)..
> >
> > I also tried it on my rig, and it does seem to take care of the problem
> >
> > Reviewed-by: Ivan Warren <[email protected]>
> > Tested-by: Ivan Warren <[email protected]>
>
> Could this patch please also be pushed for the next stable update (2.6.26
> and maybe even 2.6.25) by adding:
> CC: stable <[email protected]>
Ivan,
thanks for reporting the problem and reviewing the patch.
I've submitted the patch to our local repository, so it can
be picked up by Martin, our maintainer, for further processing.
Frans,
thanks for your suggestion. I've added a respective CC: stable
line as well.