Re: [RFC] 2.6.0 EDD enhancements

Re: [RFC] 2.6.0 EDD enhancements

[Matt Domsch]

You can import this changeset into BK by piping this whole message to
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================


ChangeSet@1.1532.1.2, 2003-12-18 16:35:16-06:00, Matt_Domsch@dell.com
  EDD: enable symlinks to SCSI devices
  
  Symlinks from /sys/firmware/edd/int13_dev8x/disc to the appropriate
  SCSI discs were added a year ago, but disabled because the
  scsi_bus list contained non-'scsi_device's at that time, which
  could have lead to an improper pointer following.    The SCSI
  mid-layer has rectified this, so this code can be re-enabled
  in edd.c once again.


 edd.c |   18 ++++--------------
 1 files changed, 4 insertions, 14 deletions


diff -Nru a/arch/i386/kernel/edd.c b/arch/i386/kernel/edd.c
--- a/arch/i386/kernel/edd.c	Fri Dec 19 12:53:18 2003
+++ b/arch/i386/kernel/edd.c	Fri Dec 19 12:53:18 2003
@@ -60,7 +60,7 @@
 MODULE_DESCRIPTION("sysfs interface to BIOS EDD information");
 MODULE_LICENSE("GPL");
 
-#define EDD_VERSION "0.11 2003-Dec-17"
+#define EDD_VERSION "0.12 2003-Dec-18"
 #define EDD_DEVICE_NAME_SIZE 16
 #define REPORT_URL "http://domsch.com/linux/edd30/results.html"
 
@@ -561,16 +561,6 @@
 	return sysfs_create_link(&edev->kobj,&pci_dev->dev.kobj,"pci_dev");
 }
 
-/*
- * FIXME - as of 15-Jan-2003, there are some non-"scsi_device"s on the
- * scsi_bus list.  The following functions could possibly mis-access
- * memory in that case.  This is actually a problem with the SCSI
- * layer, which is being addressed there.  Until then, don't use the
- * SCSI functions.
- */
-
-#undef CONFIG_SCSI
-#undef CONFIG_SCSI_MODULE
 #if defined(CONFIG_SCSI) || defined(CONFIG_SCSI_MODULE)
 
 struct edd_match_data {
@@ -639,11 +629,11 @@
 	pci_dev = edd_get_pci_dev(edev);
 	if (pci_dev) {
 		struct scsi_device * sdev = edd_find_matching_scsi_device(edev);
-		if (sdev && get_device(&sdev->sdev_driverfs_dev)) {
+		if (sdev && get_device(&sdev->sdev_gendev)) {
 			rc = sysfs_create_link(&edev->kobj,
-					       &sdev->sdev_driverfs_dev.kobj,
+					       &sdev->sdev_gendev.kobj,
 					       "disc");
-			put_device(&sdev->sdev_driverfs_dev);
+			put_device(&sdev->sdev_gendev);
 		}
 	}
 	return rc;

===================================================================


This BitKeeper patch contains the following changesets:
1.1532.1.2
## Wrapped with gzip_uu ##


M'XL( !Y)XS\  \55:8_;-A#];/V*01;8).A*(JG#1^$@[7K1+GIDL6[ZU:#)
MD<5:$@V17L>H?WR'<HX>VP!;!"@MD#8Y;_AF]!Y\ 6\=]K/13]+[U<*V3M71
M!7QOG9^--#9-HFQ+&_?6TD9:VQ;35@]AZ7J;HM9I8[K]NU@D14R_(HJ]DU[5
M\("]FXUXDGW<\<<=SD;W-]^]_?&;^RB:S^&ZEMT&E^AA/H^\[1]DH]UKZ>O&
M=HGO9>=:]#)0.'T,/0G&!'T*/LY849YXR?+Q27'-N<PY:B;R29E'?ZKG]8<Z
M_IHDXX)/!.=95IZ*\;04T0)XPHM,)#P1P+*4BY1/@)>SK)CQ,F;EC#%X+#%\
MQ2%FT;?P96NXCA3<+!8SP$ZN&P1W;*G96T?7P/)Z>0L:'XQ"1V'T+#^<5CTQ
M2MW1I97IVX/L<7A/IO,\6Q%D\B[5QJF0Q=<(<K?K[:XWTF/(,N2E8P<'[.E4
M:]0@X8BR![FQ5[#>^Q 0&&E8HY)[AR$1@0EE5NN]@\8X#\IV7IJ.HCK;Q<^'
MPS/CYPZD)TR83(M7<*@-Z4X19-]HJ.4#0H-2!XJR ],&AMC#SE(1M%:V:>S!
M=)L$:/Q"103:A&^-CAMYI)!:.NA1>5,9(N!KXZ[ V>$+W:(1%"5>(\7$Y^YJ
M@IL.J%.) MLI*GU#[)/H!PCJX-'=)[E&\1-'%#')HE>/BN<D>S*3R29ENL6^
MPR8=. P:X8Q-!>-9D9]X(<;Y:5J6TW%5C==29]58JJ=F/(M>9 4K@N@%&WSX
M>'PPY1<G_-2,[UW*IOGT)#(VR0:7"O9W?[+BL_[,(>;Y_V?004/4[#<0]X?A
M(4W<_4O?_X.Z%F4&/+H=Y@N-%;DN$%O]>G._O'WS,SQC"1<0VADO4,5\\BQ:
M%&4.G!$T%P-V6$8C4\$+1[SA\A(VZ-\[]L5EV(M?A7FUP8Z6ER_A]X#.S^A\
M0-. \_@G(-G:]6]7 5*>(>49LMM_[I:O/_VEJ!K5UNW;N2@$(J^*Z \JUFC(
$O@8     
 

Re: [RFC] 2.6.0 EDD enhancements

[James Bottomley]

On Fri, 2003-12-19 at 14:01, Matt Domsch wrote:
> @@ -639,11 +629,11 @@
>  	pci_dev = edd_get_pci_dev(edev);
>  	if (pci_dev) {
>  		struct scsi_device * sdev = edd_find_matching_scsi_device(edev);
> -		if (sdev && get_device(&sdev->sdev_driverfs_dev)) {
> +		if (sdev && get_device(&sdev->sdev_gendev)) {
>  			rc = sysfs_create_link(&edev->kobj,
> -					       &sdev->sdev_driverfs_dev.kobj,
> +					       &sdev->sdev_gendev.kobj,
>  					       "disc");
> -			put_device(&sdev->sdev_driverfs_dev);
> +			put_device(&sdev->sdev_gendev);

This is a bit nasty...you're assuming a lot of hidden knowledge about
the layout of sysfs objects in scsi_device in this code.

The current(*) way you should be doing this is to use scsi_device_get()
in your edd_match_scsi_dev() and do a scsi_device_put() after creating
the link...that should be hotplug robust.

(*) since SCSI hotplug is a work in progress, this may change...

James

Re: [RFC] 2.6.0 EDD enhancements

[Matt Domsch]

> This is a bit nasty...you're assuming a lot of hidden knowledge about
> the layout of sysfs objects in scsi_device in this code.
> 
> The current(*) way you should be doing this is to use scsi_device_get()
> in your edd_match_scsi_dev() and do a scsi_device_put() after creating
> the link...that should be hotplug robust.

Ok, I'll gladly make that change, but I still need a handle on the
sdev_gendev.kobj in order to make the symlink:

>  			rc = sysfs_create_link(&edev->kobj,
> 					       &sdev->sdev_gendev.kobj,
>  					       "disc");

While there's an accessor function to_scsi_device() to go from the
struct device to the struct scsi_device, there's not accessor to go from the
scsi_device to the struct device, which would further abstract
struct internals.  Can I get such added to a SCSI header file?
Something like:

static inline struct device *
sdev_to_gendev(struct scsi_device *sdev)
{
    return &sdev->sdev_gendev;
}


Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [RFC] 2.6.0 EDD enhancements

[James Bottomley]

On Fri, 2003-12-19 at 15:37, Matt Domsch wrote:
> Ok, I'll gladly make that change, but I still need a handle on the
> sdev_gendev.kobj in order to make the symlink:

the scsi device does its refcounting through the generic device (and
hence the kobject), so scsi_device_get() does get a handle on the
kobject for you (as well as doing some checks with the SCSI state model
and getting a handle on the underlying module).

> static inline struct device *
> sdev_to_gendev(struct scsi_device *sdev)
> {
>     return &sdev->sdev_gendev;
> }

I'm not too sure about this...exporting such a function might be seen as
encouraging further abuse...

James

Re: [RFC] 2.6.0 EDD enhancements

[Christoph Hellwig]

On Fri, Dec 19, 2003 at 01:01:29PM -0600, Matt Domsch wrote:
> ChangeSet@1.1532.1.2, 2003-12-18 16:35:16-06:00, Matt_Domsch@dell.com
>   EDD: enable symlinks to SCSI devices
>   
>   Symlinks from /sys/firmware/edd/int13_dev8x/disc to the appropriate
>   SCSI discs were added a year ago, but disabled because the
>   scsi_bus list contained non-'scsi_device's at that time, which
>   could have lead to an improper pointer following.    The SCSI
>   mid-layer has rectified this, so this code can be re-enabled
>   in edd.c once again.

As James already said you're poking far too deep into scsi internals.
In addition to his comment I'd suggest putting the symlink creating
into scsi_sysfs.c and exporting a procedural interface for the edd
code.  That way it'll get updated automatically if we change something.

Re: [RFC] 2.6.0 EDD enhancements

[Matt Domsch]

> As James already said you're poking far too deep into scsi internals.
> In addition to his comment I'd suggest putting the symlink creating
> into scsi_sysfs.c and exporting a procedural interface for the edd
> code.  That way it'll get updated automatically if we change something.

Thanks for the feedback.  I'll rework it to move to scsi_sysfs.c (and
for the similar IDE and pci_dev pieces likewise).

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com