Re: [PATCH 2/6] pata_sis: Implement MWDMA for the UDMA 133 capable chips

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Monday 07 December 2009 04:05:28 pm Bartlomiej Zolnierkiewicz wrote:
> On Monday 07 December 2009 02:26:00 pm Sergei Shtylyov wrote:
> > Hello.
> > 
> > Alan Cox wrote:
> > 
> > > Bartlomiej pointed out that while this got fixed in the old driver whoever
> > > did it didn't port it across.
> > 
> > > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > 
> > [...]
> > 
> > > +static int mwdma_clip_to_pio(struct ata_device *adev)
> > > +{
> > > +	const int mwdma_to_pio[3] = {
> > > +		XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> > > +	};
> > > +	return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
> > > +				adev->pio_mode - XFER_PIO_0);
> > > +}
> > 
> >     You call min() on uncomparables, i.e. mwdma_to_pio[] contains XFER_PIO_* 
> > and adev->pio_mode - XFER_PIO_0 yields you a mode number. Thus the second 
> > argument will always "win" as a minimal one.
> 
> There are more issues with the patch related to mwdma_clip_to_pio().
> 
> The function can return values between 0 and 4 which obviously won't work
> well for the new code below for values > 2 (i.e. resulting in out-of-bounds

Fortunately above bugs won't be triggered because the patch forgot to update
MWDMA masks..  ;)

What is even more interesting is that the driver currently also lacks support
for MWDMA modes on UDMA66 chips (though it was implemented in the driver).

static const struct ata_port_info sis_info66 = {
	.flags		= ATA_FLAG_SLAVE_POSS,
	.pio_mask	= ATA_PIO4,
	/* No MWDMA */
	.udma_mask	= ATA_UDMA4,
	.port_ops	= &sis_66_ops,
};
static const struct ata_port_info sis_info100 = {
	.flags		= ATA_FLAG_SLAVE_POSS,
	.pio_mask	= ATA_PIO4,
	/* No MWDMA */
	.udma_mask	= ATA_UDMA5,
	.port_ops	= &sis_100_ops,
};
static const struct ata_port_info sis_info100_early = {
	.flags		= ATA_FLAG_SLAVE_POSS,
	.pio_mask	= ATA_PIO4,
	/* No MWDMA */
	.udma_mask	= ATA_UDMA5,
	.port_ops	= &sis_66_ops,
};
static const struct ata_port_info sis_info133 = {
	.flags		= ATA_FLAG_SLAVE_POSS,
	.pio_mask	= ATA_PIO4,
	/* No MWDMA */
	.udma_mask	= ATA_UDMA6,
	.port_ops	= &sis_133_ops,
};
const struct ata_port_info sis_info133_for_sata = {
	.flags		= ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST,
	.pio_mask	= ATA_PIO4,
	/* No MWDMA */
	.udma_mask	= ATA_UDMA6,
	.port_ops	= &sis_133_for_sata_ops,
};
static const struct ata_port_info sis_info133_early = {
	.flags		= ATA_FLAG_SLAVE_POSS,
	.pio_mask	= ATA_PIO4,
	/* No MWDMA */
	.udma_mask	= ATA_UDMA6,
	.port_ops	= &sis_133_early_ops,
};

--
Bartlomiej Zolnierkiewicz