On Tue, 2011-06-21 at 15:30 -0500, Brian King wrote: > Looks good to me. Jeff/Tejun - any issues with merging this? BTW. Current upstream with that patch applied on a machine here leads to several oddities, I don't know at this point whether any of that is actually a regression : > Thanks, > > Brian > > On 06/21/2011 11:07 AM, Nishanth Aravamudan wrote: > > Ping on this -- Tejun, Brian, Jeff, this is a pretty annoying 2.6.39 > > regression which it would be good to have fixed in 3.0. > > > > Thanks, > > Nish > > > > On 16.06.2011 [08:28:36 -0700], Nishanth Aravamudan wrote: > >> On 16.06.2011 [08:28:39 -0500], Brian King wrote: > >>> 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. > >> > >> Commit 7b3a24c57d2eeda8dba9c205342b12689c4679f9 ("ahci: don't enable > >> port irq before handler is registered") caused a regression for CD-ROMs > >> attached to the IPR SATA bus on Power 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 FROZEN flag added in that commit is only cleared by the new EH code, > >> which is not used by ipr. Clear this flag in the SAS code if we don't > >> support new EH. > >> > >> Reported-by: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> > >> Signed-off-by: Nishanth Aravamudan <nacc@xxxxxxxxxx> > >> > >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > >> index d51f979..ebe1685 100644 > >> --- a/drivers/ata/libata-scsi.c > >> +++ b/drivers/ata/libata-scsi.c > >> @@ -3797,6 +3797,12 @@ EXPORT_SYMBOL_GPL(ata_sas_port_alloc); > >> */ > >> int ata_sas_port_start(struct ata_port *ap) > >> { > >> + /* > >> + * the port is marked as frozen at allocation time, but if we don't > >> + * have new eh, we won't thaw it > >> + */ > >> + if (!ap->ops->error_handler) > >> + ap->pflags &= ~ATA_PFLAG_FROZEN; > >> return 0; > >> } > >> EXPORT_SYMBOL_GPL(ata_sas_port_start); > >> > >> > >> -- > >> Nishanth Aravamudan <nacc@xxxxxxxxxx> > >> IBM Linux Technology Center > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html