Hi Tejun,
On Thu, Jan 10, 2013 at 06:17:01PM -0800, Tejun Heo wrote:
> Hello, Aaron.
>
> On Fri, Jan 11, 2013 at 10:11:10AM +0800, Aaron Lu wrote:
> > > What's the synchronization rule for this field?
> >
> > I documented the rule in include/scsi/scsi_device.h.
> >
> > This field is modified in the ata port's runtime suspend and resume
> > callback, and is read accessed in the check_events callback of the sr
> > block driver. The runtime PM callback is synchronized by PM core, in
> > that the two callbacks will never run concurrently. So I guess saying
> > synchronized by PM core is enough for this field?
> >
> > This is what I've added in v12 for scsi_device structure:
> >
> > + bool disable_disk_events; /* disable poll for disk events, used in
> > + * ATA layer, sychronized by PM core */
> > +
> >
> > Or do you mean I should add a comment explaining the sync rule when it
> > is modifed, like in the above code?
>
> The thing is that disabling disk events doesn't necessarily have
> anything to do with PM, so tying synchronization to PM subsystem is a
> bit unexpected. How about making it an atomic_t? That way, disabling
> can stack and synchronization dependency to PM is removed.
OK, will make it atomic in next version, thanks for the advice.
Perhaps I can add two scsi helper functions in scsi_lib.c like:
void sdev_disable_disk_events(struct scsi_device *sdev)
{
atomic_inc(&sdev->disk_events_disable_depth);
}
void sdev_enable_disk_events(struct scsi_device *sdev)
{
if (WARN_ON_ONCE(atomic_read(&sdev->disk_events_disable_depth) <= 0))
return;
atomic_dec(&sdev->disk_events_disable_depth);
}
And call them in ATA layer. Do you like this?
Thanks,
Aaron
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
