Re: [PATCH 2/3] Add disk hotswap support to libata RESEND #5

[Lukasz Kosewski]

On 9/28/05, Jeff Garzik <jgarzik@xxxxxxxxx> wrote:
> > +     dev->flags &= ~ATA_DFLAG_LBA48;
> > +
>
> probably should just clear out all the flags...

I'll look into making this cleaner :)  During testing this was the
only one that was necessary, but I'll do some clean-up foo on this.

> is ata_chk_err() call in ata_to_sense_error() the only place that needs
> to talk to the hardware?  If so, maybe we could work around that by
> making sure it is passed register values, avoiding the need to poke the h/w.

Yep, it is the only place as far as I can see.  As for passing
register values, the reason I decided not to do that is because, say,
a user unplugged his drive because there WAS some kind of error which
set ATA_ERR.  This would attempt to perform some kind of error check
which would compound the problem.  However, I agree this is dirty :)
Should I just check the register values, & ~ATA_ERR, and then pass that in?

> > +     spin_lock(&ap->host_set->lock);
> > +     hotplug_todo = ap->plug; // Make sure we don't modify while reading!
> > +     spin_unlock(&ap->host_set->lock);
>
> this lock should always be acquired using spin_lock_irqsave()

Oops.

> > +             ata_scsi_handle_unplug(ap);
> > +
> > +             // The following flag is necessary on some Seagate drives.
> > +             ap->flags |= ATA_FLAG_SATA_RESET;
>
> we can't unconditionally set this for all controllers
>
> For PATA hotplug, which this code will eventually handle, the SATA
> SControl register doesn't even exist :)

if (ap & ATA_FLAG_SATA) ap->flags |= ATA_FLAG_SATA_RESET
? :)

> > +             ap->udma_mask = ap->orig_udma_mask;
>
> dumb question -- what is this for?
>
> for hotplug we'll want to go through the entire probe sequence,
> configuring pio/dma masks all over again.

OK I'll admit I may not have looked through the code enough on that
one, but here's a situation I remember about this:
Had a drive which supported UDMA 5 max.  Unplugged it, plugged in a
drive which supported UDMA 6, but libata reported UDMA 5 max.  Since
the flags aren't reset, and ata_mode_string() works from slowest to
fastest, this will always happen unless we reset the flags to some
default value.  So... voila.  Do you have a suggestion for making this
cleaner?

> > +static void ata_hotplug_timer_func(unsigned long _data)
> > +{
> > +     struct ata_port *ap = (struct ata_port *)_data;
> > +
> > +     queue_work(ata_hotplug_wq, &ap->hotplug_task);
> > +}
>
> if this is all the timer function needs to do, then you can just use
> queue_work_delayed()

I agree.

> > +void ata_hotplug_plug(struct ata_port *ap)

<snip>

> > +void ata_hotplug_unplug(struct ata_port *ap)
>
> I prefer the names ata_hot_plug() and ata_hot_unplug()

Sure.

> > +static int ata_scsi_qc_abort(struct ata_queued_cmd *qc, u8 drv_stat)
> > +{
> > +     struct scsi_cmnd *cmd = qc->scsicmd;
> > +
> > +     cmd->result = SAM_STAT_TASK_ABORTED; //FIXME:  Is this what we want?
>
> Just trace through the error handling code, the answer should fall out
> from there...

Yeah... I put that fixme in there in August to remind myself to do
that... apparently the reminder didn't help.  I'll look into it.

> > +void ata_scsi_handle_plug(struct ata_port *ap)
> > +{
> > +     //Currently SATA only
>
> Please change comments to follow the standard style in the rest of the
> drivers:  /* foo */

Righto.

> > +void ata_scsi_handle_unplug(struct ata_port *ap)
> > +{
> > +     //SATA only, no PATA
>
> why?  This function looks generic enough to work for PATA

This just refers to the fact that I'm currently just looking for a
drive with channel, id, and lun all equal to 0.  I'll remove the
comment.

OK, looks like I have my work cut out for me.  Thanks for the feedback
Jeff, I'll resubmit this as soon as I've made changes and done some
more testing to ensure it's still stable.
-
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/