Fwd: [Patch 2.6] s390 - Allow ECKD devices to be used with older controllers and emulation

Fwd: [Patch 2.6] s390 - Allow ECKD devices to be used with older controllers and emulation

[Frans Pop]

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 (ivan@vmfacility.fr)

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

Re: Fwd: [Patch 2.6] s390 - Allow ECKD devices to be used with older controllers and emulation

[Martin Schwidefsky]

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 (ivan@vmfacility.fr)
> 
> 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.

[PATCH] S390 : Set SLI on PSF/PRSSD on Dasd ECKD initialisation

[Ivan Warren]

    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 <ivan@vmfacility.fr>
---
 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

Re: [PATCH] S390 : Set SLI on PSF/PRSSD on Dasd ECKD initialisation

[Stefan Weinhuber]

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 <wein@de.ibm.com>
---
 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));
 
 /*

Re: [PATCH] S390 : Set SLI on PSF/PRSSD on Dasd ECKD initialisation

[Ivan Warren]

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 <wein@de.ibm.com>
> ---
>  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 <ivan@vmfacility.fr>
    Tested-by: Ivan Warren <ivan@vmfacility.fr>

Thanks again !

--Ivan

Re: [PATCH] S390 : Set SLI on PSF/PRSSD on Dasd ECKD initialisation

[Frans Pop]

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 <wein@de.ibm.com>

[...]

> 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 <ivan@vmfacility.fr>
>     Tested-by: Ivan Warren <ivan@vmfacility.fr>

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 <stable@kernel.org>
?

TIA,
FJP

Re: [PATCH] S390 : Set SLI on PSF/PRSSD on Dasd ECKD initialisation

[Stefan Weinhuber]

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 <wein@de.ibm.com>
> 
> [...]
> 
> > 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 <ivan@vmfacility.fr>
> >     Tested-by: Ivan Warren <ivan@vmfacility.fr>
> 
> 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 <stable@kernel.org>

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.