Two questions on scsi_device_{get,put}

Two questions on scsi_device_{get,put}

[Christoph Hellwig]

I'm trying to forward-port my locking infrastructure for
shost->my_devices to the current scsi tree.  As this list is
acquired from irq context we need these locks to be spinlocks,
but for that to work scsi_device_get/scsi_device_put must not
block.

But after your changes it's using the class r/w semaphore for
synchronisation.  What it this protecting except sdev->access_count?
Also what's the reason we can't do the device_del directly but have
to do it in scsi_device_put?

A final nitpick:  you're losing a module refernece for some failure
pathes of scsi_module_get.

Re: Two questions on scsi_device_{get,put}

[Mike Anderson]

Please excuse the delay in reply. I was on vacation and I am just
catching up.

Christoph Hellwig [hch@lst.de] wrote:
> I'm trying to forward-port my locking infrastructure for
> shost->my_devices to the current scsi tree.  As this list is
> acquired from irq context we need these locks to be spinlocks,
> but for that to work scsi_device_get/scsi_device_put must not
> block.
> 

my_devices should be redundant now with shost_gendev.children. Will we
not have consistency issues with LDM managing a list with r/w sema and
my_devices being managed with spinlocks.

> But after your changes it's using the class r/w semaphore for
> synchronisation.  What it this protecting except sdev->access_count?

Yes, access_count is what is being protected. The r/w semaphore could be
changed to spinlock protection if needed.

> Also what's the reason we can't do the device_del directly but have
> to do it in scsi_device_put?

When we converted the upper level drivers to the driver model the probe
/ remove routines get called in response to device_add and device_delete
calls respectively. 

The problem I ran into was in sd_remove when we call del_gendisk.
del_gendisk calls delete_partition which is bad if we are still using
the block device.

I think maybe the gendisk kobject should have a release function, but
that would touch quite a few files and I had not discussed this with Al /
others.

> 
> A final nitpick:  you're losing a module refernece for some failure
> pathes of scsi_module_get.

By scsi_module_get do you mean try_module_get on the host module? I see
that I have a ref leak on failure of get_device in scsi_device_get.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Two questions on scsi_device_{get,put}

[Christoph Hellwig]

On Mon, Aug 11, 2003 at 01:37:34PM -0700, Mike Anderson wrote:
> my_devices should be redundant now with shost_gendev.children. Will we
> not have consistency issues with LDM managing a list with r/w sema and
> my_devices being managed with spinlocks.

The problem is that we need to access my_devices in IRQ context which
is impossible with the semaphore-protected shost_gendev.children.
Thus I'd prefer to use my_devices over the scsi code for consistency.

> > But after your changes it's using the class r/w semaphore for
> > synchronisation.  What it this protecting except sdev->access_count?
> 
> Yes, access_count is what is being protected. The r/w semaphore could be
> changed to spinlock protection if needed.

or atomic_t.  Or even better killed at all - if we can do the device_del
imediately it's not needed anymore.

> > Also what's the reason we can't do the device_del directly but have
> > to do it in scsi_device_put?
> 
> When we converted the upper level drivers to the driver model the probe
> / remove routines get called in response to device_add and device_delete
> calls respectively. 
> 
> The problem I ran into was in sd_remove when we call del_gendisk.
> del_gendisk calls delete_partition which is bad if we are still using
> the block device.
> 
> I think maybe the gendisk kobject should have a release function, but
> that would touch quite a few files and I had not discussed this with Al /
> others.

That sounds like the way to go.  Care to bring it up on lkml or should I?

> > A final nitpick:  you're losing a module refernece for some failure
> > pathes of scsi_module_get.
> 
> By scsi_module_get do you mean try_module_get on the host module? I see
> that I have a ref leak on failure of get_device in scsi_device_get.

Sorry, I don't have the code in front of me currently.  I'll f'up on
this once I have.