Removal of driverfs files in drivers/cdrom/cdrom.c

Removal of driverfs files in drivers/cdrom/cdrom.c

[Patrick Mochel]

So, I'm looking at changing the driverfs API. In doring so, I came across 
this code in drivers/cdrom/cdrom.c:unregister_cdrom:

        if (atomic_read (&cdi->cdrom_driverfs_dev.refcount)) {
                device_remove_file (&cdi->cdrom_driverfs_dev, "name");
                device_remove_file (&cdi->cdrom_driverfs_dev, "kdev");
                put_device (&cdi->cdrom_driverfs_dev);
        }

This looks wrong, based on the fact that it wasn't the cdrom layer that 
created these files. They were, I believe, created by the SCSI cdrom 
layer. So, why are they being removed here, esp. since they are removed at 
a more general layer? 

Thanks,

        -pat

Re: Removal of driverfs files in drivers/cdrom/cdrom.c

[sullivan]

On Wed, Jul 24, 2002 at 02:57:11PM -0700, Patrick Mochel wrote:
> 
> 
> So, I'm looking at changing the driverfs API. In doring so, I came across 
> this code in drivers/cdrom/cdrom.c:unregister_cdrom:
> 
>         if (atomic_read (&cdi->cdrom_driverfs_dev.refcount)) {
>                 device_remove_file (&cdi->cdrom_driverfs_dev, "name");
>                 device_remove_file (&cdi->cdrom_driverfs_dev, "kdev");
>                 put_device (&cdi->cdrom_driverfs_dev);
>         }
> 
> This looks wrong, based on the fact that it wasn't the cdrom layer that 
> created these files. They were, I believe, created by the SCSI cdrom 
> layer. So, why are they being removed here, esp. since they are removed at 
> a more general layer? 

Yes, I struggled with this one. I would prefer the implementation where the
lower layer (ie. scsi, ide) setup only the parent and bus fields of the
cdrom_driverfs_dev structure and let the more general layer (cdrom.c) do the
rest as part of the register_cdrom() call. Similarily, unregister_cdrom() would
do the removal.

Since there are a lot of drivers that register with the general cdrom layer 
that don't support driverfs yet, I didn't want to make changes across a whole
bunch of components needlessly until people got a chance to look at what the
device tree might look like for the scsi implementation. 

Maybe a first good step would be to move the cdrom support completely up into
the general layer and use the presence of a non NULL parent field to indicate
whether the lower level driver has added support for driverfs. We'd have to
make sure the cdrom_drverfs_dev structure is initialized with zeros for all of
the cdrom related driver layers that don't support driverfs yet. Let me know
what you think. I'll pull a patch together... 

> 
> Thanks,
> 
>         -pat
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Removal of driverfs files in drivers/cdrom/cdrom.c

[James Bottomley]

mochel@osdl.org said:
> This looks wrong, based on the fact that it wasn't the cdrom layer
> that  created these files. They were, I believe, created by the SCSI
> cdrom  layer. So, why are they being removed here, esp. since they are
> removed at  a more general layer?  

sullivan@austin.ibm.com said:
> Maybe a first good step would be to move the cdrom support completely
> up into the general layer and use the presence of a non NULL parent
> field to indicate whether the lower level driver has added support for
> driverfs. We'd have to make sure the cdrom_drverfs_dev structure is
> initialized with zeros for all of the cdrom related driver layers that
> don't support driverfs yet. Let me know what you think. I'll pull a
> patch together...  

Pat,

I think the root cause of all the ugly code is that none of us knows how to 
match up the partitions or other adjunct elements which belong in the class 
code with the actual device, and the fact that the generic code (cdrom and 
genhd) is more than just SCSI, so we're all a little wary of messing too much 
with it.  Are you planning to embed a driverfs parent pointer in the relevant 
structures?  Do you think the best approach is to do driverfs creation/removal 
for everything in the generic cdrom driver?  We're all a bit reticent to do a 
wholesale rework of none scsi code without a clear idea of what we're supposed 
to be doing.

If you give me an idea of how you are thinking of matching up the parent 
pointers, I'll have a go at doing this more generally.

James

Re: Removal of driverfs files in drivers/cdrom/cdrom.c

[Patrick Mochel]

> I think the root cause of all the ugly code is that none of us knows how to 
> match up the partitions or other adjunct elements which belong in the class 
> code with the actual device, and the fact that the generic code (cdrom and 
> genhd) is more than just SCSI, so we're all a little wary of messing too much 
> with it.  Are you planning to embed a driverfs parent pointer in the relevant 
> structures?  Do you think the best approach is to do driverfs creation/removal 
> for everything in the generic cdrom driver?  We're all a bit reticent to do a 
> wholesale rework of none scsi code without a clear idea of what we're supposed 
> to be doing.
> 
> If you give me an idea of how you are thinking of matching up the parent 
> pointers, I'll have a go at doing this more generally.

I wasn't attacking the code per se; it just looked wrong that a completely 
different layer was removing the files than was adding them...

I'm not sure I understand your question. You already have a parent 
pointer in struct device. Is that what you're referring to, or do you need 
something more?

As far as what to do with the SCSI and cdrom-specific stuff: you may be 
unclear, but you have a far better idea than I. So, I'm leaving it in your 
hands. And, don't try and suck me in. ;)

	-pat