libata/ipr/powerpc: regression between 2.6.39-rc4 and 2.6.39-rc5

Hi Jeff,

It appears that commit 7b3a24c57d2eeda8dba9c205342b12689c4679f9 broke
CD-ROMs on the IPR SATA bus on powerpc machines:

 ata_port_alloc: ENTER
 ata_port_probe: ata1: bus probe begin
 ata1.00: ata_dev_read_id: ENTER
 ata1.00: failed to IDENTIFY (I/O error, err_mask=0x40)
 ata1.00: ata_dev_read_id: ENTER
 ata1.00: failed to IDENTIFY (I/O error, err_mask=0x40)
 ata1.00: limiting speed to UDMA7:PIO5
 ata1.00: ata_dev_read_id: ENTER
 ata1.00: failed to IDENTIFY (I/O error, err_mask=0x40)
 ata1.00: disabled
 ata_port_probe: ata1: bus probe end
 scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured

The commit in question is tagged as AHCI, but does touch the libata
core. I'm not well-versed with this code, but here's what I think I know
:)

From __scsi_add_device, we first allocate the target:

 scsi_alloc_target ->
  shost->hostt->target_alloc ->
   ipr_target_alloc ->
    ata_port_alloc ->
     sets FROZEN

We error out when we allocate the sdev:

 scsi_alloc_sdev ->
  shost->hostt->slave_alloc ->
   ipr_slave_alloc ->
    ipr_ata_slave_alloc ->
     ata_sas_port_init ->
      ata_port_probe

From what I can tell, the only place that explicitly clears the FROZEN
flag is the error-handling code via ata_eh_thaw_port().

So I thought either we're not invoking the error-handler at probe time
correctly to kick the port or perhaps the SAS code is not clearing the
flag?

I tried the following patch:


and the CD-ROM drive works, but I have no idea if it's the right thing
to do. I chose this particular change, FWIW, because we call
ata_sas_port_start before we probe in ata_sas_port_init and it seems
like we need to mark the port as not frozen before we init it? Perhaps
that should really be a call to a thaw function, not sure. Let's just
say the ATA/SAS/SCSI interactions are a bit hard to follow at first :)

Any insight you can provide would be great!

Thanks,
Nish

On 06/15/2011 02:17 PM, Nishanth Aravamudan wrote:
> From what I can tell, the only place that explicitly clears the FROZEN
> flag is the error-handling code via ata_eh_thaw_port().
> 
> So I thought either we're not invoking the error-handler at probe time
> correctly to kick the port or perhaps the SAS code is not clearing the
> flag?
> 
> I tried the following patch:
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d51f979..abd0e0b 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3797,6 +3797,7 @@ EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
>   */
>  int ata_sas_port_start(struct ata_port *ap)
>  {
> +       ap->pflags &= ~ATA_PFLAG_FROZEN;
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(ata_sas_port_start);
> 
> and the CD-ROM drive works, but I have no idea if it's the right thing
> to do. I chose this particular change, FWIW, because we call
> ata_sas_port_start before we probe in ata_sas_port_init and it seems
> like we need to mark the port as not frozen before we init it? Perhaps
> that should really be a call to a thaw function, not sure. Let's just
> say the ATA/SAS/SCSI interactions are a bit hard to follow at first :)

That looks like the right thing to do. For ipr's usage of
libata, we don't have the concept of a port frozen state, so this flag
should really never get set. The alternate way to fix this would be to
only set ATA_PFLAG_FROZEN in ata_port_alloc if ap->ops->error_handler
is not NULL.

Thanks,

Brian

On 15.06.2011 [15:02:18 -0500], Brian King wrote:
> On 06/15/2011 02:17 PM, Nishanth Aravamudan wrote:
> > From what I can tell, the only place that explicitly clears the FROZEN
> > flag is the error-handling code via ata_eh_thaw_port().
> > 
> > So I thought either we're not invoking the error-handler at probe time
> > correctly to kick the port or perhaps the SAS code is not clearing the
> > flag?
> > 
> > I tried the following patch:
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index d51f979..abd0e0b 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -3797,6 +3797,7 @@ EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
> >   */
> >  int ata_sas_port_start(struct ata_port *ap)
> >  {
> > +       ap->pflags &= ~ATA_PFLAG_FROZEN;
> >         return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(ata_sas_port_start);
> > 
> > and the CD-ROM drive works, but I have no idea if it's the right thing
> > to do. I chose this particular change, FWIW, because we call
> > ata_sas_port_start before we probe in ata_sas_port_init and it seems
> > like we need to mark the port as not frozen before we init it? Perhaps
> > that should really be a call to a thaw function, not sure. Let's just
> > say the ATA/SAS/SCSI interactions are a bit hard to follow at first :)
> 
> That looks like the right thing to do. For ipr's usage of
> libata, we don't have the concept of a port frozen state, so this flag
> should really never get set. The alternate way to fix this would be to
> only set ATA_PFLAG_FROZEN in ata_port_alloc if ap->ops->error_handler
> is not NULL.

It seemed like ipr is as you say, but I wasn't sure if it was
appropriate to make the change above in the common libata-scis code or
not. I don't want to break some other device on accident.

Also, I tried your suggestion, but I don't think that can happen in
ata_port_alloc? ata_port_alloc is allocated ap itself, and it seems like
ap->ops typically gets set only after ata_port_alloc returns?

Thanks,
Nish

On Wed, Jun 15, 2011 at 04:34:17PM -0700, Nishanth Aravamudan wrote:
> > That looks like the right thing to do. For ipr's usage of
> > libata, we don't have the concept of a port frozen state, so this flag
> > should really never get set. The alternate way to fix this would be to
> > only set ATA_PFLAG_FROZEN in ata_port_alloc if ap->ops->error_handler
> > is not NULL.
> 
> It seemed like ipr is as you say, but I wasn't sure if it was
> appropriate to make the change above in the common libata-scis code or
> not. I don't want to break some other device on accident.
> 
> Also, I tried your suggestion, but I don't think that can happen in
> ata_port_alloc? ata_port_alloc is allocated ap itself, and it seems like
> ap->ops typically gets set only after ata_port_alloc returns?

Maybe we can test error_handler in ata_sas_port_start()?

Thanks.

On 06/16/2011 02:51 AM, Tejun Heo wrote:
> On Wed, Jun 15, 2011 at 04:34:17PM -0700, Nishanth Aravamudan wrote:
>>> That looks like the right thing to do. For ipr's usage of
>>> libata, we don't have the concept of a port frozen state, so this flag
>>> should really never get set. The alternate way to fix this would be to
>>> only set ATA_PFLAG_FROZEN in ata_port_alloc if ap->ops->error_handler
>>> is not NULL.
>>
>> It seemed like ipr is as you say, but I wasn't sure if it was
>> appropriate to make the change above in the common libata-scis code or
>> not. I don't want to break some other device on accident.
>>
>> Also, I tried your suggestion, but I don't think that can happen in
>> ata_port_alloc? ata_port_alloc is allocated ap itself, and it seems like
>> ap->ops typically gets set only after ata_port_alloc returns?
> 
> Maybe we can test error_handler in ata_sas_port_start()?

Good point. Since libsas is converted to the new eh now, we would need to have
this test.

Thanks,

Brian