libata-core.c: ata_fill_sg() ... sanity check on arithmentic ?

libata-core.c: ata_fill_sg() ... sanity check on arithmentic ?

[MikeW]

Could someone more knowledgeable than me please reassure me ...

Referring to code in linux-2.6.22.1: libata-core.c (kernel.org),
in ata_fill_sg():

if sg_dma_address(sg) is 64k-aligned and sg_dma_len(sg) is 64k (0x10000),
then in the while loop, the "if ((offset..." test is false, since
it tests for *greater than* 64k

So then ap->prd[idx].flags_len gets set to (0x10000 & 0xFFFF) = 0 !

So this sg element ends up with a zero length, even though the
transfer size should be 64k.

Is this correct behaviour, if not, should it be corrected ?
Sorry if I'm just being paranoid !

Thanks,
MikeW

Re: libata-core.c: ata_fill_sg() ... sanity check on arithmentic ?

[Alan Cox]

> So then ap->prd[idx].flags_len gets set to (0x10000 & 0xFFFF) = 0 !
> So this sg element ends up with a zero length, even though the
> transfer size should be 64k.
> 
> Is this correct behaviour, if not, should it be corrected ?

The specification says that 0x0000 means 64K. A couple of controllers do
get this wrong and we did recently add a second sg list builder for those.

Perhaps a comment or two is in order ?

Re: libata-core.c: ata_fill_sg() ... sanity check on arithmentic ?

[MikeW]

Alan Cox <alan <at> lxorguk.ukuu.org.uk> writes:

> 
> > So then ap->prd[idx].flags_len gets set to (0x10000 & 0xFFFF) = 0 !
> > So this sg element ends up with a zero length, even though the
> > transfer size should be 64k.
> > 
> > Is this correct behaviour, if not, should it be corrected ?
> 
> The specification says that 0x0000 means 64K. A couple of controllers do
> get this wrong and we did recently add a second sg list builder for those.
> 
> Perhaps a comment or two is in order ?
> -

Thanks - but which specification is this ?

As far as I am aware there is no spec for DMA controllers running ATA.

Since DMA controller details may vary according to hardware, I
would have expected that ata_fill_sg() would be one of the overloadable
function entries with a driver-specific function that knew about
the DMA controller niceties, and how to stuff a sg entry.

Cheers,
MikeW

Re: libata-core.c: ata_fill_sg() ... sanity check on arithmentic ?

[Alan Cox]

> > The specification says that 0x0000 means 64K. A couple of controllers do
> > get this wrong and we did recently add a second sg list builder for those.
> > 
> > Perhaps a comment or two is in order ?
> > -
> 
> Thanks - but which specification is this ?
> 
> As far as I am aware there is no spec for DMA controllers running ATA.

SFF 8038i covers the standard taskfile IDE class PCI device interface for
PIO and DMA. It also covers the alternate ADMA interface.

> Since DMA controller details may vary according to hardware, I
> would have expected that ata_fill_sg() would be one of the overloadable
> function entries with a driver-specific function that knew about
> the DMA controller niceties, and how to stuff a sg entry.

It is -n ot directly however but by overloading qc_prep. That makes more
sense than overloading fill_sg as not all controllers fit a simple
SFF like scatter/gather list model in the first place.

Alan

Re: libata-core.c: ata_fill_sg() ... sanity check on arithmentic ?

[MikeW]

Alan Cox <alan <at> lxorguk.ukuu.org.uk> writes:

> 
>>> The specification says that 0x0000 means 64K. A couple of controllers do
>>> get this wrong and we did recently add a second sg list builder for those.
>>> 
>>> Perhaps a comment or two is in order ?
>>> -
>> 
>> Thanks - but which specification is this ?
>> 
>> As far as I am aware there is no spec for DMA controllers running ATA.
> 
> SFF 8038i covers the standard taskfile IDE class PCI device interface for
> PIO and DMA. It also covers the alternate ADMA interface.
> 
>> Since DMA controller details may vary according to hardware, I
>> would have expected that ata_fill_sg() would be one of the overloadable
>> function entries with a driver-specific function that knew about
>> the DMA controller niceties, and how to stuff a sg entry.
> 
> It is -n ot directly however but by overloading qc_prep. That makes more
> sense than overloading fill_sg as not all controllers fit a simple
> SFF like scatter/gather list model in the first place.
> 
> Alan
> -

Thanks for clarifying - as you say, the spec is specific to PCI.
Our silicon has its own host interface which was confused by a length of zero !
Overloading qc_prep seems the way to go here.
As you say, some comments may be in order !

Regards,
MikeW

Re: libata-core.c: ata_fill_sg() ... SATA AHCI

[MikeW]

Alan Cox <alan <at> lxorguk.ukuu.org.uk> writes:

>
> SFF 8038i covers the standard taskfile IDE class PCI device interface for
> PIO and DMA. It also covers the alternate ADMA interface.
>
>> Since DMA controller details may vary according to hardware, I
>> would have expected that ata_fill_sg() would be one of the overloadable
>> function entries with a driver-specific function that knew about
>> the DMA controller niceties, and how to stuff a sg entry.
>
> It is not directly however but by overloading qc_prep. That makes more
> sense than overloading fill_sg as not all controllers fit a simple
> SFF like scatter/gather list model in the first place.
>
> Alan
> -

I note also that Intel's Serial ATA AHCI spec has a 22-bit DMA byte count
in the PRDT, so 0 == 64k cannot be assumed here either.

Cheers,
MikeW

Re: libata-core.c: ata_fill_sg() ... sanity check on arithmentic ?

[Alan Cox]

> Thanks for clarifying - as you say, the spec is specific to PCI.
> Our silicon has its own host interface which was confused by a length of zero !

You aren't the first. If you look at the current tree you'll see we now
have

	ata_dumb_qc_prep()

for interfaces that think 0x0000 is zero. Set that as your qc_prep method
and set sg_tablesize in the driver to LIBATA_DUMB_MAX_PRD and it should
work (we have to use a smaller sglist limit as the worst case is now 2
entries per 64K block)

Re: libata-core.c: ata_fill_sg() ... SATA AHCI

[Alan Cox]

> I note also that Intel's Serial ATA AHCI spec has a 22-bit DMA byte count
> in the PRDT, so 0 == 64k cannot be assumed here either.

Different standard and our AHCI driver doesn't use ata_fill_sg either 8)