Re: libata-core: devslp fails on ATP mSATA

Re: libata-core: devslp fails on ATP mSATA

[Tejun Heo]

(cc'ing Hannes and Margin)

Hello,

On Tue, Dec 01, 2015 at 10:22:34AM +0100, Andreas Werner wrote:
> Hi,
> i have a question regarding the devslp feature in several SATA devices.
> 
> I currently working on an embedded board with a Freescale P1013/P1022.
> I have a mSATA Device from ATP which is connected to the SATA port.
> The Yocot Kernel 3.14.26 is used in the system.
> 
> This mSata failed with the following log messages:
...
> The device is not recognized by the System, and i do not have access to it.
> 
> I have also tested the mSATA using a 3.14 Kernel and a 4.2 Kernel on
> a Intel Atom E3825. On this system, the mSATA is working perfectly.
> 
> Next test was to use a 3.4.18 Kernel (which was the old BSP) on the P1013.
> With this Kernel the mSATA is also working correctly.
> 
> 
> During some deeper debuggin in libata-core.c i find that the problem is
> regarding the devslp.
> 
> /* Check and mark DevSlp capability. Get DevSlp timing variables
> * from SATA Settings page of Identify Device Data Log.
> */
> if (ata_id_has_devslp(dev->id)) {
> u8 *sata_setting = ap->sector_buf;
> int i, j;
> 
> dev->flags |= ATA_DFLAG_DEVSLP;
> err_mask = ata_read_log_page(dev,
>         ATA_LOG_SATA_ID_DEV_DATA,
>         ATA_LOG_SATA_SETTINGS,
>         sata_setting,
>         1);
> if (err_mask)
>  ata_dev_dbg(dev,
>       "failed to get Identify Device Data, Emask 0x%x\n",
>       err_mask);
> else
>  for (i = 0; i < ATA_LOG_DEVSLP_SIZE; i++) {
>   j = ATA_LOG_DEVSLP_OFFSET + i;
>   dev->devslp_timing[i] = sata_setting[j];
>  }
> }
> 
> 
> If i comment out this code, the device is recognized correctly. I figured out
> that only the "ata_read_log_page" return a AC_ERR_DEV, and makes the device
> malfunction (-> failed to set xfermode).
> 
> I am not that familiar with SATA and did not understand why the "ata_read_log_page" can
> corrupt my mSATA device.
> 
> The only thing what i can imagine is, that the Host controller does not support devslp
> (which is the case).
> 
> Does anybody of you have hint for me to fixe that problem in a clean way?

So, I suppose this is a fallout from 9faa643855df ("libata: use
READ_LOG_DMA_EXT").  The description just says "we should try use it"
but is there any benefit from using NCQ variant of read log page?  I
mean, it's used during config and error handling, so it's not like
queueing is gonna help anything.  It ends up issuing NCQ commands on
controllers which don't support NCQ and causes failures on devices
which lie about supporting the feature.

Thanks.

-- 
tejun

Re: libata-core: devslp fails on ATP mSATA

[Tejun Heo]

On Tue, Dec 01, 2015 at 11:17:15AM -0500, Tejun Heo wrote:
> So, I suppose this is a fallout from 9faa643855df ("libata: use
> READ_LOG_DMA_EXT").  The description just says "we should try use it"
> but is there any benefit from using NCQ variant of read log page?  I
> mean, it's used during config and error handling, so it's not like
> queueing is gonna help anything.  It ends up issuing NCQ commands on
> controllers which don't support NCQ and causes failures on devices
> which lie about supporting the feature.

Hmmm... so, the controller locks up on READ_LOG_EXT issued as pio?
That's just weird.  I suppose you can blacklist the controller so that
any attempt to read the log page fails without actually issuing the
command.

Thanks.

-- 
tejun

Re: libata-core: devslp fails on ATP mSATA

[Andreas Werner]

On Tue, Dec 01, 2015 at 02:04:10PM -0500, Tejun Heo wrote:
> On Tue, Dec 01, 2015 at 11:17:15AM -0500, Tejun Heo wrote:
> > So, I suppose this is a fallout from 9faa643855df ("libata: use
> > READ_LOG_DMA_EXT").  The description just says "we should try use it"
> > but is there any benefit from using NCQ variant of read log page?  I
> > mean, it's used during config and error handling, so it's not like
> > queueing is gonna help anything.  It ends up issuing NCQ commands on
> > controllers which don't support NCQ and causes failures on devices
> > which lie about supporting the feature.
> 
> Hmmm... so, the controller locks up on READ_LOG_EXT issued as pio?
> That's just weird.  I suppose you can blacklist the controller so that
> any attempt to read the log page fails without actually issuing the
> command.
> 
> Thanks.
> 
> -- 
> tejun

Hi,
yes since i am using 3.14 where the READ_LOG_DMA_EXT is not it, it
seems that the controller locks up even on READ_LOG_EXT as PIO.

I have checked the current ERRATA sheet of the CPU but there
are no issues regarding this problem.

Test on a Freescale T4240 show the same behaviour as on P1013/P1022.
It seems that some FSL Cpus are affected.
I have asked my FAE at Freescale if he has some information about
that kind of issue.

Blacklisting the controller would be a solution yes, but I just
wanna wait the answer from the FAE to be sure we really have a
problem.

What kind of identifier can I use for blacklisting? The driver name
from the host driver? (fsl-sata) I did not find any register
in the sata controller to read out an ID.

Regards
Andy

Re: libata-core: devslp fails on ATP mSATA

[Tejun Heo]

Hello, Andreas.

On Wed, Dec 02, 2015 at 10:33:10AM +0100, Andreas Werner wrote:
> Blacklisting the controller would be a solution yes, but I just
> wanna wait the answer from the FAE to be sure we really have a
> problem.
> 
> What kind of identifier can I use for blacklisting? The driver name
> from the host driver? (fsl-sata) I did not find any register
> in the sata controller to read out an ID.

Whatever matches the affected controllers.  If all sata_fsl
controllers are affected, the driver init code can set the flag
unconditionally.

Thanks.

-- 
tejun

Re: libata-core: devslp fails on ATP mSATA

[Andreas Werner]

On Wed, Dec 02, 2015 at 11:47:53AM -0500, Tejun Heo wrote:
> Hello, Andreas.
> 
> On Wed, Dec 02, 2015 at 10:33:10AM +0100, Andreas Werner wrote:
> > Blacklisting the controller would be a solution yes, but I just
> > wanna wait the answer from the FAE to be sure we really have a
> > problem.
> > 
> > What kind of identifier can I use for blacklisting? The driver name
> > from the host driver? (fsl-sata) I did not find any register
> > in the sata controller to read out an ID.
> 
> Whatever matches the affected controllers.  If all sata_fsl
> controllers are affected, the driver init code can set the flag
> unconditionally.
> 
> Thanks.
> 
> -- 
> tejun

Ok, setting the flag in the controller driver would be good.
(still wating for the FAE for more info).

As I can see the horkage field is only defind in "struct ata_dev"
would it be time to add a horkage field to "struct ata_host"?

All the other "flag" fields in the structs are used and/or reserved
and it seems to be no good place for such flags.

What I am thinking about is.

1. Add new flag e.g. ATA_HORKAGE_NOLOG_PAGE_RD
2. Add horkage field to ata_host struct
2. Set this flag in ata_host struct in the sata_fsl driver (init)
3. Copy the controller horkage flags over to struct ata_device in
   the ata_dev_configure function in libata

   At the end all flags set by the controller are applied to
   the ata device horkage flags, and can be used for blacklisting
   in libata.


May be there are better solution, or i am missing something.

What do you think about it?

Regards
Andy	

Re: libata-core: devslp fails on ATP mSATA

[Tejun Heo]

Hello,

On Thu, Dec 03, 2015 at 10:33:55AM +0100, Andreas Werner wrote:
> All the other "flag" fields in the structs are used and/or reserved
> and it seems to be no good place for such flags.

You can use the port flags - ATA_FLAG_*.

> What I am thinking about is.
> 
> 1. Add new flag e.g. ATA_HORKAGE_NOLOG_PAGE_RD

ATA_FLAG_NO_LOG_PAGE?

Thanks.

-- 
tejun

Re: libata-core: devslp fails on ATP mSATA

[Andreas Werner]

On Thu, Dec 03, 2015 at 10:15:49AM -0500, Tejun Heo wrote:
> Hello,
> 
> On Thu, Dec 03, 2015 at 10:33:55AM +0100, Andreas Werner wrote:
> > All the other "flag" fields in the structs are used and/or reserved
> > and it seems to be no good place for such flags.
> 
> You can use the port flags - ATA_FLAG_*.
> 
> > What I am thinking about is.
> > 
> > 1. Add new flag e.g. ATA_HORKAGE_NOLOG_PAGE_RD
> 
> ATA_FLAG_NO_LOG_PAGE?

That seems to be much more simpler than my idea :-)
Did not see the port stuff, i was focused on the host struct.

Thank you for the hint. I will prepare a Patch next week.

> 
> Thanks.
> 
> -- 
> tejun

Regards
Andy