Re: [git patches] libata fixes

[David Daney]

Andrew Morton wrote:
> On Fri, 16 Jan 2009 10:27:21 -0500 Jeff Garzik <jeff@xxxxxxxxxx> wrote:

> > +/**
> > + * Convert nanosecond based time to setting used in the
> > + * boot bus timing register, based on timing multiple
> > + */
>
> There are comments in this file which use the kerneldoc token, but
> which aren't in kerneldoc format.

I will endeavor to improve it.

> > +static unsigned int ns_to_tim_reg(unsigned int tim_mult, unsigned int nsecs)
> > +{
> > +	unsigned int val;
> > +
> > +	/*
> > +	 * Compute # of eclock periods to get desired duration in
> > +	 * nanoseconds.
> > +	 */
> > +	val = DIV_ROUND_UP(nsecs * (octeon_get_clock_rate() / 1000000),
> > +			  1000 * tim_mult);
> > +
> > +	return val;
> > +}
>
> There's great potential for overflows here, but I couldn't be bothered
> picking through it.  Are we sure that it's watertight?

We are calculating timing register values, there will not be overflow
given that the delays are all under 1000nS and the clock rate will
never be greater than 10GHz.


> There's a 64-bit divide in there.  Will it link on 32-bit platforms?

It might not...

> Or is this all 64-bit-only code?

... but we are currently only supporting 64 bit kernels.


> wtf is an octeon anyway?  (greps).  Some MIPS thing.

Multicore MIPS64 SOC.

> I guess it's 64-bit-only.

Current, yes.  See above.

[...]
> > +/**
> > + * Called after libata determines the needed PIO mode. This
> > + * function programs the Octeon bootbus regions to support the
> > + * timing requirements of the PIO mode.
> > + *
> > + * @ap:     ATA port information
> > + * @dev:    ATA device
> > + */
>
> That's getting more kerneldoccy, but isn't there yet.

As with the others, I will try to improve it.

[...]
> > +	T = (int)(2000000000000LL / octeon_get_clock_rate());
>
> 64/64 divide.

As with the case above, it works as only 64 bit kernel supported.

[...]
> > +static void octeon_cf_tf_read16(struct ata_port *ap, struct ata_taskfile *tf)
> > +{
> > +	u16 blob;
> > +	/* The base of the registers is at ioaddr.data_addr. */
> > +	void __iomem *base = ap->ioaddr.data_addr;
> > +
> > +	blob = __raw_readw(base + 0xc);
>
> why __raw?

readw() does byte swapping, we don't want that, hence the __raw.

[...]
> > +
> > +	cf_port = (struct octeon_cf_port *)ap->private_data;
>
> Unneeded, undesirable cast of void* (multiple instances).

I will fix it.


[...]
> > +static irqreturn_t octeon_cf_interrupt(int irq, void *dev_instance)
> > +{
> > +	struct ata_host *host = dev_instance;
> > +	struct octeon_cf_port *cf_port;
> > +	int i;
> > +	unsigned int handled = 0;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&host->lock, flags);
>
> Would spin_lock() suffice here?

I have to think about that one.



Thanks for looking at it,
David daney

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/