sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Martin Michlmayr]

With 2.6.26-rc1 I have to enable CONFIG_SATA_PMP=y otherwise sata_mv
doesn't find a disk on my Orion based device (QNAP TS-209).  Without
CONFIG_SATA_PMP I get:

[42949381.790000] sata_mv sata_mv.0: version 1.20
[42949381.790000] sata_mv sata_mv.0: slots 32 ports 2
[42949381.800000] scsi0 : sata_mv
[42949381.800000] scsi1 : sata_mv
[42949381.810000] ata1: SATA max UDMA/133 irq 29
[42949381.810000] ata2: SATA max UDMA/133 irq 29
[42949382.170000] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[42949382.520000] ata2: SATA link down (SStatus 0 SControl 300)
[and nothing else]

So obviously it sees that something is connected to ata1, but it
doesn't actually print anything about the disk or create the device
node.

This works correctly with 2.6.25 or when I set CONFIG_SATA_PMP=y.
However, my device doesn't have a multiplier.
-- 
Martin Michlmayr
http://www.cyrius.com/

Re: sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Mark Lord]

Martin Michlmayr wrote:
> With 2.6.26-rc1 I have to enable CONFIG_SATA_PMP=y otherwise sata_mv
> doesn't find a disk on my Orion based device (QNAP TS-209).  Without
> CONFIG_SATA_PMP I get:
> 
> [42949381.790000] sata_mv sata_mv.0: version 1.20
> [42949381.790000] sata_mv sata_mv.0: slots 32 ports 2
> [42949381.800000] scsi0 : sata_mv
> [42949381.800000] scsi1 : sata_mv
> [42949381.810000] ata1: SATA max UDMA/133 irq 29
> [42949381.810000] ata2: SATA max UDMA/133 irq 29
> [42949382.170000] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> [42949382.520000] ata2: SATA link down (SStatus 0 SControl 300)
> [and nothing else]
> 
> So obviously it sees that something is connected to ata1, but it
> doesn't actually print anything about the disk or create the device
> node.
> 
> This works correctly with 2.6.25 or when I set CONFIG_SATA_PMP=y.
> However, my device doesn't have a multiplier.
..

Okay, so the "SStatus 123" means that sata_mv::mv_hardreset must
have succeeded in finding good link status.  Great.

After that, libata-core and pals do the actual device IDENTIFY/probing.

With CONFIG_SATA_PMP=y, there's an added soft-reset done first,
and without it, there is no soft-reset.

Looks like your device either needs the extra soft-reset regardless,
or maybe just needs a little more time in the probing sequence.

Tejun?

I suppose we could just always ask for the extra soft-reset here (?)

Re: sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Tejun Heo]

Mark Lord wrote:
> Martin Michlmayr wrote:
>> With 2.6.26-rc1 I have to enable CONFIG_SATA_PMP=y otherwise sata_mv
>> doesn't find a disk on my Orion based device (QNAP TS-209).  Without
>> CONFIG_SATA_PMP I get:
>>
>> [42949381.790000] sata_mv sata_mv.0: version 1.20
>> [42949381.790000] sata_mv sata_mv.0: slots 32 ports 2
>> [42949381.800000] scsi0 : sata_mv
>> [42949381.800000] scsi1 : sata_mv
>> [42949381.810000] ata1: SATA max UDMA/133 irq 29
>> [42949381.810000] ata2: SATA max UDMA/133 irq 29
>> [42949382.170000] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>> [42949382.520000] ata2: SATA link down (SStatus 0 SControl 300)
>> [and nothing else]
>>
>> So obviously it sees that something is connected to ata1, but it
>> doesn't actually print anything about the disk or create the device
>> node.
>>
>> This works correctly with 2.6.25 or when I set CONFIG_SATA_PMP=y.
>> However, my device doesn't have a multiplier.
> ..
> 
> Okay, so the "SStatus 123" means that sata_mv::mv_hardreset must
> have succeeded in finding good link status.  Great.
> 
> After that, libata-core and pals do the actual device IDENTIFY/probing.
> 
> With CONFIG_SATA_PMP=y, there's an added soft-reset done first,
> and without it, there is no soft-reset.
> 
> Looks like your device either needs the extra soft-reset regardless,
> or maybe just needs a little more time in the probing sequence.
> 
> Tejun?
> 
> I suppose we could just always ask for the extra soft-reset here (?)

Hmm... sata_mv doesn't do device classification, right?  If so, it 
should return -EAGAIN on success to tell EH to perform follow-up SRST.

Thanks.

-- 
tejun

Re: sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> I suppose we could just always ask for the extra soft-reset here (?)
> 
> Hmm... sata_mv doesn't do device classification, right?  If so, it 
> should return -EAGAIN on success to tell EH to perform follow-up SRST.
..

Right now, mv_hardreset() is just a wrapper around sata_link_hardreset(),
and it just returns whatever it gets back from sata_link_hardreset().

And sata_link_hardreset() only returns -EAGAIN for hosts that support PMP.

Should sata_link_hardreset() instead always return -EAGAIN instead of 0 ?
Because that's essentially what we'd be doing by changing mv_hardreset()
to do that.

???

Re: sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
> ..
>>> I suppose we could just always ask for the extra soft-reset here (?)
>>
>> Hmm... sata_mv doesn't do device classification, right?  If so, it 
>> should return -EAGAIN on success to tell EH to perform follow-up SRST.
> ..
> 
> Right now, mv_hardreset() is just a wrapper around sata_link_hardreset(),
> and it just returns whatever it gets back from sata_link_hardreset().
> 
> And sata_link_hardreset() only returns -EAGAIN for hosts that support PMP.
> 
> Should sata_link_hardreset() instead always return -EAGAIN instead of 0 ?
> Because that's essentially what we'd be doing by changing mv_hardreset()
> to do that.
...

Mmm.. or maybe sata_mv should do what sata_std_hardreset() does:

   rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
   return online ? -EAGAIN : rc;

Or, for that matter, perhaps sata_mv should just be calling sata_std_hardreset()
instead of sata_link_hardreset().   Those all got changed around at the same time
that I was working on sata_mv, so I might not have it quite right there (?).

Cheers

Re: sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Mark Lord]

Mark Lord wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Mark Lord wrote:
>> ..
>>>> I suppose we could just always ask for the extra soft-reset here (?)
>>>
>>> Hmm... sata_mv doesn't do device classification, right?  If so, it 
>>> should return -EAGAIN on success to tell EH to perform follow-up SRST.
>> ..
>>
>> Right now, mv_hardreset() is just a wrapper around sata_link_hardreset(),
>> and it just returns whatever it gets back from sata_link_hardreset().
>>
>> And sata_link_hardreset() only returns -EAGAIN for hosts that support 
>> PMP.
>>
>> Should sata_link_hardreset() instead always return -EAGAIN instead of 0 ?
>> Because that's essentially what we'd be doing by changing mv_hardreset()
>> to do that.
> ...
> 
> Mmm.. or maybe sata_mv should do what sata_std_hardreset() does:
> 
>   rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
>   return online ? -EAGAIN : rc;
> 
> Or, for that matter, perhaps sata_mv should just be calling sata_std_hardreset()
> instead of sata_link_hardreset().   Those all got changed around at the same time
> that I was working on sata_mv, so I might not have it quite right there (?).
..

Ahh. Okay, I see what's going on.

Patch coming shortly.

-ml

Re: sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Mark Lord]

Martin Michlmayr wrote:
> With 2.6.26-rc1 I have to enable CONFIG_SATA_PMP=y otherwise sata_mv
> doesn't find a disk on my Orion based device (QNAP TS-209).  Without
> CONFIG_SATA_PMP I get:
> 
> [42949381.790000] sata_mv sata_mv.0: version 1.20
> [42949381.790000] sata_mv sata_mv.0: slots 32 ports 2
> [42949381.800000] scsi0 : sata_mv
> [42949381.800000] scsi1 : sata_mv
> [42949381.810000] ata1: SATA max UDMA/133 irq 29
> [42949381.810000] ata2: SATA max UDMA/133 irq 29
> [42949382.170000] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> [42949382.520000] ata2: SATA link down (SStatus 0 SControl 300)
> [and nothing else]
> 
> So obviously it sees that something is connected to ata1, but it
> doesn't actually print anything about the disk or create the device
> node.
> 
> This works correctly with 2.6.25 or when I set CONFIG_SATA_PMP=y.
> However, my device doesn't have a multiplier.
..

Okay, does this fix it for you?
Please reply after testing to let us all know,
and then I'll repost the patch for Jeff / upstream-linus.

-------------- snip ---------------

Always request a softreset after hardreset succeeds.
This fixes a regression reported by Martin Michlmayr <tbm@cyrius.com>.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-05-09 17:21:52.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-05-13 18:10:29.000000000 -0400
@@ -2728,6 +2728,7 @@
 
 		rc = sata_link_hardreset(link, timing, deadline + extra,
 					 &online, NULL);
+		rc = online ? -EAGAIN : rc;
 		if (rc)
 			return rc;
 		sata_scr_read(link, SCR_STATUS, &sstatus);

Re: sata_mv on Orion: CONFIG_SATA_PMP=y needed even when no multiplier

[Martin Michlmayr]

* Mark Lord <liml@rtr.ca> [2008-05-13 18:55]:
> Okay, does this fix it for you?
> Please reply after testing to let us all know,

Yes, this works.  Thanks.

Tested-by: Martin Michlmayr <tbm@cyrius.com>

-- 
Martin Michlmayr
http://www.cyrius.com/