[PATCHv2] aacraid: don't assign cpu_to_le32(constant) to u8
Noticed on PowerPC allmod config build: drivers/scsi/aacraid/commsup.c:1342: warning: large integer implicitly truncated to unsigned type drivers/scsi/aacraid/commsup.c:1343: warning: large integer implicitly truncated to unsigned type drivers/scsi/aacraid/commsup.c:1344: warning: large integer implicitly truncated to unsigned type Also fix some whitespace on the changed lines. Signed-off-by: Stephen Rothwell <[EMAIL PROTECTED]> --- drivers/scsi/aacraid/commsup.c |6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) This version just fixes a couple of whitespace anomolies on the lines I changed. -- Cheers, Stephen Rothwell[EMAIL PROTECTED] diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c index 240a0bb..3c2dbc0 100644 --- a/drivers/scsi/aacraid/commsup.c +++ b/drivers/scsi/aacraid/commsup.c @@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac) aif = (struct aac_aifcmd *)hw_fib->data; aif->command = cpu_to_le32(AifCmdEventNotify); aif->seqnum = cpu_to_le32(0x); - aif->data[0] = cpu_to_le32(AifEnExpEvent); - aif->data[1] = cpu_to_le32(AifExeFirmwarePanic); - aif->data[2] = cpu_to_le32(AifHighPriority); + aif->data[0] = AifEnExpEvent; + aif->data[1] = AifExeFirmwarePanic; + aif->data[2] = AifHighPriority; aif->data[3] = cpu_to_le32(BlinkLED); /* -- 1.5.3.4 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] add DID_REQUEUE string to scsi_show_result host table
I was working on patches which add new transport error values, when I
noticed that DID_REQUEUE was not in the hostbyte_table. I do not think
there is any way to hit the code path where scsi_show_result is called
and where you return DID_REQUEUE, because DID_REQUEUE causes scsi-ml to
always requeue the command. However, for completeness and because I want
to one day send a patch that tries to add new host bytes values, I am
sending this patch.
Please apply when you get a chance. It is not critical.
Signed-off-by: Mike Christie <[EMAIL PROTECTED]>
--- linux-2.6.23/drivers/scsi/constants.c.orig 2007-10-31 20:49:47.0
-0500
+++ linux-2.6.23/drivers/scsi/constants.c 2007-10-31 20:50:14.0
-0500
@@ -1355,7 +1355,7 @@ EXPORT_SYMBOL(scsi_print_sense);
static const char * const hostbyte_table[]={
"DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT", "DID_BAD_TARGET",
"DID_ABORT", "DID_PARITY", "DID_ERROR", "DID_RESET", "DID_BAD_INTR",
-"DID_PASSTHROUGH", "DID_SOFT_ERROR", "DID_IMM_RETRY"};
+"DID_PASSTHROUGH", "DID_SOFT_ERROR", "DID_IMM_RETRY", "DID_REQUEUE"};
#define NUM_HOSTBYTE_STRS ARRAY_SIZE(hostbyte_table)
static const char * const driverbyte_table[]={
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
DMA API Re: Remove dma_coherent_mem interface
Hi folks. Please CC me - I am NOT on LKML at present There was a recent patch to remove the coherent memory interface from the DMA API. I'd like to ask that this not happen - there are several users of this code in the handhelds.org tree, which we hope will merge into mainline. this code is _essential_ to allow support of these devices. Removing this from mainline will only increase the hh.org delta which we are working hard to reduce, and there is no other realistic way to support these devices. Thanks, Ian - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: DMA API Re: Remove dma_coherent_mem interface
On Wed, Oct 31, 2007 at 09:03:24PM +, ian wrote: > There was a recent patch to remove the coherent memory interface from > the DMA API. > > I'd like to ask that this not happen - there are several users of this > code in the handhelds.org tree, which we hope will merge into mainline. > this code is _essential_ to allow support of these devices. > > Removing this from mainline will only increase the hh.org delta which we > are working hard to reduce, and there is no other realistic way to > support these devices. It's been three years since this API was introduced, and the only *merged* user is the one that James wrote. Now it's in the way of some performance improvements I want to make. I can't possibly go round every little platform tree looking for users -- you need to get your drivers into mainline. Yes, it's not necessarily a quick process, but equally, you've had three years. Now, what driver is it that's going to consume this coherent memory? Does it use the dma_pool interface, or does it use dma_alloc_coherent directly? If the latter, I can retain the dma_declare_coherent_memory interface. If the former, then we have to come to a decision between supporting some drivers that are so unimportant that they haven't been merged in three years and making a performance win for drivers that are used every day. Admittedly, I haven't finished the work to make dma_pool use slub, thus I don't know what the performance win will turn out to be. It may not be significant, so it would be an easy decision to keep the interface. But if the decision is going to be to keep the interface despite the lack of users and no matter what the performance win, then I don't need to finish this work. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] aacraid: thread stop patch
Got a panic in the threading code on an older kernel when the Adapter
failed to load properly and driver shut down apparently before any
threading had started, can not dupe. Expect that this may be relevant in
the latest kernel, but not sure. This patch does no harm, and should
alleviate the possibility of this panic.
This attached patch is against current scsi-misc-2.6.
ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patch attachments (inline gets damaged, use attachment).
Signed-off-by: Mark Salyzyn <[EMAIL PROTECTED]>
drivers/scsi/aacraid/linit.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff -ru a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
--- a/drivers/scsi/aacraid/linit.c 2007-10-31 16:32:10.394394185
-0400
+++ b/drivers/scsi/aacraid/linit.c 2007-10-31 16:32:17.740458222
-0400
@@ -959,7 +959,8 @@
static void __aac_shutdown(struct aac_dev * aac)
{
- kthread_stop(aac->thread);
+ if (aac->aif_thread)
+ kthread_stop(aac->thread);
aac_send_shutdown(aac);
aac_adapter_disable_int(aac);
free_irq(aac->pdev->irq, aac);
Sincerely -- Mark Salyzyn
aacraid_thread_stop.patch
Description: aacraid_thread_stop.patch
Re: slow after upgrade to CentOS 5 (RHEL5)
James Bottomley wrote: > Please don't drop the cc lists. There are others who probably have more > informed opinions than I do who won't get to comment if they don't see > it. > > On Sun, 2007-10-28 at 17:36 -0700, Anthony Ewell wrote: >> James Bottomley wrote: >>> On Sat, 2007-10-27 at 13:21 -0700, Anthony Ewell wrote: James Bottomley wrote: > On Wed, 2007-10-24 at 18:04 -0700, Anthony Ewell wrote: >> Hi All, >> >> If you all would not mind a post from the general >> public Linux user, after doing a complete disk wipe >> of CentOS 4 and installing CentOS5, my system is preceived >> to be 3 times slower. >> >> To troubleshooting this, I made a post on CentOS's >> bugzilla: http://bugs.centos.org/view.php?id=2382 >> >> Would some of the experts on this group mind >> looking at the bug to evaluate the possibility >> that it is being caused by the underlying scsi >> driver? The post contains a dmesg from "Computer C". >> (Yes, I am getting a bit desperate.) > There's still too little information in the bug report to tell much of > anything. The dmesg doesn't indicate any anomaly with the megaraid > (although the LSI people might be able to tell better). However, it > also doesn't contain a trace of the tape drive. > > Best guess would be a slow down in the megaraid driver. Can you try > doing a speed test on it? (hdparm -t should suffice). > > James Hi James, The other guy reporting the problem http://www.centos.org/modules/newbb/viewtopic.php?topic_id=10659&start=0#forumpost34209 is not using a MegaRAID card. He is using 3ware9508 Raid Controllers. He is also using a different processor (amd vs xeon) and a different chipset (Intel Greenwood vs nVidia) I also spoke to Neela Kolli (Mega RAID maintainer) and he said he'd never heard of the problem. Here are some tests (including dhparm) that I sent to Neela (he never wrote back). I have also checked with Stellen over at the "dump" list and he has not seen the problem (yet). The problem occurs when backing up to a two different types of tape drives and to an eSata drive. When I am running a "dump" on computer C, gnome-system-monitor shows my two cores running at only about 10 to 20% and switching back and forth (one at 0% the other at 20% for about 5 seconds, then switching positions) On Computer C (Cent OS 5), when typing in Word Pro (a windows word processor) in Parallels, I can watch myself type. Computer B (CentOS 4.4, now 4.5) has the same version of Parallels installed on it (Parallels-2.2.2112-lin.i386) that computer C (CentOS 5) has. The perceived speed difference is about a factor of three (you can not watch yourself type). All the "Low Level" test I run seem to come out the same between Cent OS 4.4 and 5. Very frustrating! It is almost like some system monitor component is looking at everything and slowing things down. If this was Windows, I'd go straight to the Anti Virus as the culprit. (Does SE Linux do such things?) Are there any performance tests I can run for you? Thank you for letting me ramble, this problem is really frustrating. I am afraid to any additional CentOS5 server out there and CentOS 4.x is so terribly out of date. -T ~~ Tests I sent to Neela: CentOS 5 (2.6.18-8.1.8.el5, Sata150-4): #grep -i bogomips /var/log/dmesg Calibrating delay using timer specific routine.. 4001.91 BogoMIPS (lpj=2000959) Calibrating delay using timer specific routine.. 3999.58 BogoMIPS (lpj=1999791) Total of 2 processors activated (8001.50 BogoMIPS). #/sbin/hdparm -t /dev/sda /dev/sda: Timing buffered disk reads: 236 MB in 3.01 seconds = 78.53 MB/sec #/sbin/hdparm -t /dev/sdb /dev/sdb: Timing buffered disk reads: 182 MB in 3.01 seconds = 60.37 MB/sec ~~ CentOS 4.4 (linux rescue 2.6.9-42.EL, IDE): #cat /proc/cpuinfo bogomips: 4002.92 #/sbin/hdparm -t /dev/sda /dev/sda: Timing buffered disk reads: 216 MB in 3.01 seconds = 71.87 MB/sec #/sbin/hdparm -t /dev/sdb /dev/sdb: Timing buffered disk reads: 184 MB in 3.01 seconds = 61.18 MB/sec >>> That pretty much shows, if anything, that transfer speed improved >>> from .9 to .18. >>> ~~ CentOS 5 (2.6.18-8.1.3.el5, Sata300-4): #grep -i bogomips /var/log/dmesg Calibrating delay using timer specific routine.. 4001.92 BogoMIPS (lpj=2000960) Calibrating delay using timer specific rout
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 31 Oct 2007, James Bottomley wrote: > > Dropping a kobject's reference to its parent when the kobject is > > removed rather than when it is deleted will solve all these problems. > > The queue's reference to the gendisk and the gendisk's reference to the > > device will both be dropped when the device is unregistered, allowing > > the device and gendisk to be released. When the device is released it > > can then drop its reference to the queue, allowing the queue to be > > released. > > > > In other words, reverting that 4-year-old patch would fix everything. > > I'd still like to get your acknowledgement for my patch, posted here: > > > > http://marc.info/?l=linux-scsi&m=119368368904151&w=2 > > It sounds plausible, but I'm not really up to speed on all the changes > going on in sysfs at the moment, so it would be dangerous to rely on > what I think sounds reasonable. Okay. It sounds like there's not much SCSI-specific remaining in this thread, so anything further should move back to LKML. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 31 Oct 2007, James Bottomley wrote: > But you're assuming the relationship gendisk->queue exists when we > delete the scsi device. What should happen is that the gendisk should > be released first in the ULD detach. Queues are allowed to exist > without gendisks (when a SCSI queue is first created, that's what it > should look like), so I think we're perhaps simply missing an API that > would detach the queue and the gendisk (and get the gendisk to delete > itself and give up its device link) ... doesn't del_gendisk() do this? It tries to, but it fails. What happens is del_gendisk() calls unlink_gendisk(), which calls blk_unregister_queue(). If the kobject core worked rationally, this would cause the queue to drop its reference to the gendisk and allow the gendisk to be released. But it doesn't. The kobject core doesn't drop the reference from a child to a parent when the child is unregistered; it waits until the child is released. This means that the gendisk can't be released until the queue is released. Similarly, the scsi_device can't be released until the gendisk is released. And obviously this constitutes a deadly cycle. The proposed patch will make the kobject core behave as it should, dropping references during unregister instead of waiting until release. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
On Wed, Oct 31, 2007 at 09:38:04AM -0500, James Bottomley wrote: > On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote: > > OSame problem here, if grp->is_visible is not set, sysfs_add_file() would > > never be called, right? > > > > Other than the logic problem (I think), I have no issue with this idea > > at all. Care to redo this so it works? > > It's a fair cop govenor ... new patch attached. Much better, thanks :) I have no problem with this, if you want to take this in your tree, please do and feel free to add: Signed-off-by: Greg Kroah-Hartman <[EMAIL PROTECTED]> to it, or I can take it in mine. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 11:46 -0500, James Bottomley wrote: > On Wed, 2007-10-31 at 17:42 +0100, Kay Sievers wrote: > > On Wed, 2007-10-31 at 11:31 -0500, James Bottomley wrote: > > > Doesn't this circularity now exist for everything? Every device that > > > creates a queue has a reference to the queue, every queue has a > > > reference to its attached gendisk and now every gendisk has a reference > > > to the device creating the queue? This doesn't look to be a SCSI > > > specific problem. > > > > It's only SCSI so far, everything else seems fine. > > But you didn't respond to the stated problem: there are very few other > non-scsi block devices ... have you tried looking at cciss? I tried ub instead of usb-storage and sd card driver which is a raw block driver, i didn't try cciss, or other more advanced blockdev drivers. > > But, the real problem is that the core seems to deadlock if two devices > > reference each other (or build a larger circle), even when they are > > deleted, that's the problem we are running in. > > Yes ... we sort of evaded that one by having class devices hang off the > sides of real devices ... making everything a peer with crosslinks > brings it back with a vengeance. Yeah, which made sysfs a horrible mess as soon as people started putting hierarchy in class devices. The whole idea of "physical", "virtual", "class", "bus" causes nothing but problems in userspace, and exposes kernel implementation details, which change all the time. There are even subsystem which moved from class to bus because they need driver binding. And there is really no point in exposing that to userspace. It's a design mistake, we try to fix now. We really need a single classification and a single hierarchy and not the 3 completely different access rules for 3 types of devices, which don't have any interesting difference, besides the way the kernel handles them. Kay - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 12:44 -0400, Alan Stern wrote: > On Wed, 31 Oct 2007, James Bottomley wrote: > > > OK, light beginning to go on now. > > > > The problem is that you've fallen into the conceptual trap we tried very > > hard to avoid in the initial go around of joining SCSI upper layer > > drivers to gendisks. That's why no gendisk references are held by the > > mid-layer, only by the entities that represent the objects created by > > upper layer drivers. > > > > Doesn't this circularity now exist for everything? Every device that > > creates a queue has a reference to the queue, every queue has a > > reference to its attached gendisk and now every gendisk has a reference > > to the device creating the queue? This doesn't look to be a SCSI > > specific problem. > > It probably isn't. I haven't looked at other subsystems but the > scenario you described could well be the common case. > > Dropping a kobject's reference to its parent when the kobject is > removed rather than when it is deleted will solve all these problems. > The queue's reference to the gendisk and the gendisk's reference to the > device will both be dropped when the device is unregistered, allowing > the device and gendisk to be released. When the device is released it > can then drop its reference to the queue, allowing the queue to be > released. > > In other words, reverting that 4-year-old patch would fix everything. > I'd still like to get your acknowledgement for my patch, posted here: > > http://marc.info/?l=linux-scsi&m=119368368904151&w=2 It sounds plausible, but I'm not really up to speed on all the changes going on in sysfs at the moment, so it would be dangerous to rely on what I think sounds reasonable. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/8] scsi: megaraid_sas - add hibernation support
On Wed, 2007-10-31 at 07:25 -0600, Yang, Bo wrote: > James, > I did respond to Randy earlier. Sorry, I couldn't find it in the archive when I looked ... although the scsi reflector seems to be missing quite a few of your emails for some reason. > About the module params being added : > > The fast_load parameter is for the user to decide at driver load time > if (s)he wants to skip scan of devices in PD channels. > > After driver is loaded the user cannot be permitted to modify this > value. If the user needs to see the devices in the PD channels, (s)he > may initiate a scan via sysfs/proc based on the kernel being used. > Once the user has done the scan, the fast_load value does not have any > significance and thus not exposed for reading. > > cmd_per_lun & max_sectors are also intended to be provided by user > only at driver load time. In the current implementation both these do > appear as read-only values under host# in sysfs. The current design is > not to allow these values to be modified on the fly. fastload sounds reasonable. cmd_per_lun and max_sectors could reasonably be altered at runtime ... just execute and unbind and rebind operation to make them take effect. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 31 Oct 2007, James Bottomley wrote: > OK, light beginning to go on now. > > The problem is that you've fallen into the conceptual trap we tried very > hard to avoid in the initial go around of joining SCSI upper layer > drivers to gendisks. That's why no gendisk references are held by the > mid-layer, only by the entities that represent the objects created by > upper layer drivers. > > Doesn't this circularity now exist for everything? Every device that > creates a queue has a reference to the queue, every queue has a > reference to its attached gendisk and now every gendisk has a reference > to the device creating the queue? This doesn't look to be a SCSI > specific problem. It probably isn't. I haven't looked at other subsystems but the scenario you described could well be the common case. Dropping a kobject's reference to its parent when the kobject is removed rather than when it is deleted will solve all these problems. The queue's reference to the gendisk and the gendisk's reference to the device will both be dropped when the device is unregistered, allowing the device and gendisk to be released. When the device is released it can then drop its reference to the queue, allowing the queue to be released. In other words, reverting that 4-year-old patch would fix everything. I'd still like to get your acknowledgement for my patch, posted here: http://marc.info/?l=linux-scsi&m=119368368904151&w=2 Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 17:42 +0100, Kay Sievers wrote:
> On Wed, 2007-10-31 at 11:31 -0500, James Bottomley wrote:
> > On Wed, 2007-10-31 at 17:24 +0100, Kay Sievers wrote:
> > > On Wed, 2007-10-31 at 11:13 -0500, James Bottomley wrote:
> > > > On Wed, 2007-10-31 at 12:04 -0400, Alan Stern wrote:
> > > > > On Wed, 31 Oct 2007, James Bottomley wrote:
> > > > >
> > > > > > > Yes, the queue is a child of the disk.
> > > > > >
> > > > > > Right, so this goes gendisk->queue (-> meaning parent of, or takes
> > > > > > reference to)
> > > > >
> > > > > No, no! The _child_ takes an implicit reference to the _parent_, not
> > > > > the other way around.
> > > > >
> > > > > > > > The scsi_device has a ref to the queue
> > > > > > >
> > > > > > > Yeah, while the queue is a grandchild of the scsi_device with the
> > > > > > > unified sysfs layout.
> > > > > >
> > > > > > No, the scsi_device is a direct parent of the queue, so we have
> > > > > >
> > > > > > scsi_device->queue
> > > > >
> > > > > Wrong -- the gendisk is the direct parent of the queue. The relevant
> > > > > line is in ll_rw_blk.c:blk_register_queue():
> > > > >
> > > > > q->kobj.parent = kobject_get(&disk->dev.kobj);
> > > > >
> > > > > > > Yes, sounds right. We need to break that
> > > > > > > deleted-but-wait-for-cleanup at
> > > > > > > least at one of the devices involved.
> > > > > >
> > > > > > But it's broken when the driver is unbound. Diagrammatically it's:
> > > > > >
> > > > > > scsi_disk -> scsi_device -> queue
> > > > > > -> gendisk ->
> > > > > >
> > > > > > It's not circular, it's released when scsi_disk is released. It can
> > > > > > become circular if there's some hidden dependency between any of the
> > > > > > components ... but I don't think there is.
> > > > >
> > > > > Forget about the scsi_disk. It isn't part of the problem. Just
> > > > > concentrate on the scsi_device, the gendisk, and the queue. We have:
> > > > >
> > > > > scsi_device <- gendisk <- queue <- scsi_device,
> > > >
> > > > OK, so where does the gendisk get a reference to the scsi device?
> > >
> > > In the unified sysfs layout where the silly and conceptual broken idea
> > > of "class devices" gets removed.
> > > Everything that has a "device" link today will just live below the
> > > device the "device" link points to. The whole current kernel is already
> > > converted to do this, besides the "raw kobject" gendisk's, and the SCSI
> > > subsystem. The gendisk patch is queued in Greg's tree (see subject of
> > > this mail), and the conversion from "struct class_device" to "struct
> > > device" for the whole SCSI directory is coming soon.
> > >
> > > With the gendisk pointing to "driverfs_dev" ("device" link) it will
> > > become a child of the scsi_device.
> >
> > OK, light beginning to go on now.
> >
> > The problem is that you've fallen into the conceptual trap we tried very
> > hard to avoid in the initial go around of joining SCSI upper layer
> > drivers to gendisks. That's why no gendisk references are held by the
> > mid-layer, only by the entities that represent the objects created by
> > upper layer drivers.
>
> That will not change, only the disk will reference the device which it
> points to. It's not a problem, we can "orphan" the disk on delete, or we
> do the "orphaning" for all devices in the core, which is probably the
> right fix anyway.
But you're assuming the relationship gendisk->queue exists when we
delete the scsi device. What should happen is that the gendisk should
be released first in the ULD detach. Queues are allowed to exist
without gendisks (when a SCSI queue is first created, that's what it
should look like), so I think we're perhaps simply missing an API that
would detach the queue and the gendisk (and get the gendisk to delete
itself and give up its device link) ... doesn't del_gendisk() do this?
> > Doesn't this circularity now exist for everything? Every device that
> > creates a queue has a reference to the queue, every queue has a
> > reference to its attached gendisk and now every gendisk has a reference
> > to the device creating the queue? This doesn't look to be a SCSI
> > specific problem.
>
> It's only SCSI so far, everything else seems fine.
But you didn't respond to the stated problem: there are very few other
non-scsi block devices ... have you tried looking at cciss?
> But, the real problem is that the core seems to deadlock if two devices
> reference each other (or build a larger circle), even when they are
> deleted, that's the problem we are running in.
Yes ... we sort of evaded that one by having class devices hang off the
sides of real devices ... making everything a peer with crosslinks
brings it back with a vengeance.
James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 11:31 -0500, James Bottomley wrote:
> On Wed, 2007-10-31 at 17:24 +0100, Kay Sievers wrote:
> > On Wed, 2007-10-31 at 11:13 -0500, James Bottomley wrote:
> > > On Wed, 2007-10-31 at 12:04 -0400, Alan Stern wrote:
> > > > On Wed, 31 Oct 2007, James Bottomley wrote:
> > > >
> > > > > > Yes, the queue is a child of the disk.
> > > > >
> > > > > Right, so this goes gendisk->queue (-> meaning parent of, or takes
> > > > > reference to)
> > > >
> > > > No, no! The _child_ takes an implicit reference to the _parent_, not
> > > > the other way around.
> > > >
> > > > > > > The scsi_device has a ref to the queue
> > > > > >
> > > > > > Yeah, while the queue is a grandchild of the scsi_device with the
> > > > > > unified sysfs layout.
> > > > >
> > > > > No, the scsi_device is a direct parent of the queue, so we have
> > > > >
> > > > > scsi_device->queue
> > > >
> > > > Wrong -- the gendisk is the direct parent of the queue. The relevant
> > > > line is in ll_rw_blk.c:blk_register_queue():
> > > >
> > > > q->kobj.parent = kobject_get(&disk->dev.kobj);
> > > >
> > > > > > Yes, sounds right. We need to break that
> > > > > > deleted-but-wait-for-cleanup at
> > > > > > least at one of the devices involved.
> > > > >
> > > > > But it's broken when the driver is unbound. Diagrammatically it's:
> > > > >
> > > > > scsi_disk -> scsi_device -> queue
> > > > > -> gendisk ->
> > > > >
> > > > > It's not circular, it's released when scsi_disk is released. It can
> > > > > become circular if there's some hidden dependency between any of the
> > > > > components ... but I don't think there is.
> > > >
> > > > Forget about the scsi_disk. It isn't part of the problem. Just
> > > > concentrate on the scsi_device, the gendisk, and the queue. We have:
> > > >
> > > > scsi_device <- gendisk <- queue <- scsi_device,
> > >
> > > OK, so where does the gendisk get a reference to the scsi device?
> >
> > In the unified sysfs layout where the silly and conceptual broken idea
> > of "class devices" gets removed.
> > Everything that has a "device" link today will just live below the
> > device the "device" link points to. The whole current kernel is already
> > converted to do this, besides the "raw kobject" gendisk's, and the SCSI
> > subsystem. The gendisk patch is queued in Greg's tree (see subject of
> > this mail), and the conversion from "struct class_device" to "struct
> > device" for the whole SCSI directory is coming soon.
> >
> > With the gendisk pointing to "driverfs_dev" ("device" link) it will
> > become a child of the scsi_device.
>
> OK, light beginning to go on now.
>
> The problem is that you've fallen into the conceptual trap we tried very
> hard to avoid in the initial go around of joining SCSI upper layer
> drivers to gendisks. That's why no gendisk references are held by the
> mid-layer, only by the entities that represent the objects created by
> upper layer drivers.
That will not change, only the disk will reference the device which it
points to. It's not a problem, we can "orphan" the disk on delete, or we
do the "orphaning" for all devices in the core, which is probably the
right fix anyway.
> Doesn't this circularity now exist for everything? Every device that
> creates a queue has a reference to the queue, every queue has a
> reference to its attached gendisk and now every gendisk has a reference
> to the device creating the queue? This doesn't look to be a SCSI
> specific problem.
It's only SCSI so far, everything else seems fine.
But, the real problem is that the core seems to deadlock if two devices
reference each other (or build a larger circle), even when they are
deleted, that's the problem we are running in.
Thanks,
Kay
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 17:24 +0100, Kay Sievers wrote:
> On Wed, 2007-10-31 at 11:13 -0500, James Bottomley wrote:
> > On Wed, 2007-10-31 at 12:04 -0400, Alan Stern wrote:
> > > On Wed, 31 Oct 2007, James Bottomley wrote:
> > >
> > > > > Yes, the queue is a child of the disk.
> > > >
> > > > Right, so this goes gendisk->queue (-> meaning parent of, or takes
> > > > reference to)
> > >
> > > No, no! The _child_ takes an implicit reference to the _parent_, not
> > > the other way around.
> > >
> > > > > > The scsi_device has a ref to the queue
> > > > >
> > > > > Yeah, while the queue is a grandchild of the scsi_device with the
> > > > > unified sysfs layout.
> > > >
> > > > No, the scsi_device is a direct parent of the queue, so we have
> > > >
> > > > scsi_device->queue
> > >
> > > Wrong -- the gendisk is the direct parent of the queue. The relevant
> > > line is in ll_rw_blk.c:blk_register_queue():
> > >
> > > q->kobj.parent = kobject_get(&disk->dev.kobj);
> > >
> > > > > Yes, sounds right. We need to break that deleted-but-wait-for-cleanup
> > > > > at
> > > > > least at one of the devices involved.
> > > >
> > > > But it's broken when the driver is unbound. Diagrammatically it's:
> > > >
> > > > scsi_disk -> scsi_device -> queue
> > > > -> gendisk ->
> > > >
> > > > It's not circular, it's released when scsi_disk is released. It can
> > > > become circular if there's some hidden dependency between any of the
> > > > components ... but I don't think there is.
> > >
> > > Forget about the scsi_disk. It isn't part of the problem. Just
> > > concentrate on the scsi_device, the gendisk, and the queue. We have:
> > >
> > > scsi_device <- gendisk <- queue <- scsi_device,
> >
> > OK, so where does the gendisk get a reference to the scsi device?
>
> In the unified sysfs layout where the silly and conceptual broken idea
> of "class devices" gets removed.
> Everything that has a "device" link today will just live below the
> device the "device" link points to. The whole current kernel is already
> converted to do this, besides the "raw kobject" gendisk's, and the SCSI
> subsystem. The gendisk patch is queued in Greg's tree (see subject of
> this mail), and the conversion from "struct class_device" to "struct
> device" for the whole SCSI directory is coming soon.
>
> With the gendisk pointing to "driverfs_dev" ("device" link) it will
> become a child of the scsi_device.
OK, light beginning to go on now.
The problem is that you've fallen into the conceptual trap we tried very
hard to avoid in the initial go around of joining SCSI upper layer
drivers to gendisks. That's why no gendisk references are held by the
mid-layer, only by the entities that represent the objects created by
upper layer drivers.
Doesn't this circularity now exist for everything? Every device that
creates a queue has a reference to the queue, every queue has a
reference to its attached gendisk and now every gendisk has a reference
to the device creating the queue? This doesn't look to be a SCSI
specific problem.
James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 11:13 -0500, James Bottomley wrote:
> On Wed, 2007-10-31 at 12:04 -0400, Alan Stern wrote:
> > On Wed, 31 Oct 2007, James Bottomley wrote:
> >
> > > > Yes, the queue is a child of the disk.
> > >
> > > Right, so this goes gendisk->queue (-> meaning parent of, or takes
> > > reference to)
> >
> > No, no! The _child_ takes an implicit reference to the _parent_, not
> > the other way around.
> >
> > > > > The scsi_device has a ref to the queue
> > > >
> > > > Yeah, while the queue is a grandchild of the scsi_device with the
> > > > unified sysfs layout.
> > >
> > > No, the scsi_device is a direct parent of the queue, so we have
> > >
> > > scsi_device->queue
> >
> > Wrong -- the gendisk is the direct parent of the queue. The relevant
> > line is in ll_rw_blk.c:blk_register_queue():
> >
> > q->kobj.parent = kobject_get(&disk->dev.kobj);
> >
> > > > Yes, sounds right. We need to break that deleted-but-wait-for-cleanup at
> > > > least at one of the devices involved.
> > >
> > > But it's broken when the driver is unbound. Diagrammatically it's:
> > >
> > > scsi_disk -> scsi_device -> queue
> > > -> gendisk ->
> > >
> > > It's not circular, it's released when scsi_disk is released. It can
> > > become circular if there's some hidden dependency between any of the
> > > components ... but I don't think there is.
> >
> > Forget about the scsi_disk. It isn't part of the problem. Just
> > concentrate on the scsi_device, the gendisk, and the queue. We have:
> >
> > scsi_device <- gendisk <- queue <- scsi_device,
>
> OK, so where does the gendisk get a reference to the scsi device?
In the unified sysfs layout where the silly and conceptual broken idea
of "class devices" gets removed.
Everything that has a "device" link today will just live below the
device the "device" link points to. The whole current kernel is already
converted to do this, besides the "raw kobject" gendisk's, and the SCSI
subsystem. The gendisk patch is queued in Greg's tree (see subject of
this mail), and the conversion from "struct class_device" to "struct
device" for the whole SCSI directory is coming soon.
With the gendisk pointing to "driverfs_dev" ("device" link) it will
become a child of the scsi_device.
Kay
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 12:04 -0400, Alan Stern wrote: > On Wed, 31 Oct 2007, James Bottomley wrote: > > > > Yes, the queue is a child of the disk. > > > > Right, so this goes gendisk->queue (-> meaning parent of, or takes > > reference to) > > No, no! The _child_ takes an implicit reference to the _parent_, not > the other way around. > > > > > The scsi_device has a ref to the queue > > > > > > Yeah, while the queue is a grandchild of the scsi_device with the > > > unified sysfs layout. > > > > No, the scsi_device is a direct parent of the queue, so we have > > > > scsi_device->queue > > Wrong -- the gendisk is the direct parent of the queue. The relevant > line is in ll_rw_blk.c:blk_register_queue(): > > q->kobj.parent = kobject_get(&disk->dev.kobj); > > > > Yes, sounds right. We need to break that deleted-but-wait-for-cleanup at > > > least at one of the devices involved. > > > > But it's broken when the driver is unbound. Diagrammatically it's: > > > > scsi_disk -> scsi_device -> queue > > -> gendisk -> > > > > It's not circular, it's released when scsi_disk is released. It can > > become circular if there's some hidden dependency between any of the > > components ... but I don't think there is. > > Forget about the scsi_disk. It isn't part of the problem. Just > concentrate on the scsi_device, the gendisk, and the queue. We have: > > scsi_device <- gendisk <- queue <- scsi_device, OK, so where does the gendisk get a reference to the scsi device? James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 11:58 -0400, Alan Stern wrote: > On Wed, 31 Oct 2007, James Bottomley wrote: > > > On Wed, 2007-10-31 at 07:32 -0700, Greg KH wrote: > > > Hm, I seem to have missed the part in this thread where someone said > > > that it was valid to have a parent reference a child device. That's > > > just wrong and needs to be fixed. Is that in the scsi layer somewhere? > > > The block layer? It sure isn't in the driver core... > > > > This is the piece I'm still not clear on. It's something to do with the > > gendisk. I'd have to look in block, but I believe the queue takes a ref > > to the gendisk. > > > > The scsi_device has a ref to the queue and the scsi_disk (in sd) has a > > ref to both the scsi_device and the gendisk. That means, until sd is > > unbound and the scsi_disk released, there's an implied unbreakable > > reference chain. > > > > at least, I think that's what the problem is. > > No, you haven't got it right. > > Parent Child Grandchild > -- - -- > scsi_device gendisk request_queue This is what I think doesn't happen. The scsi_device should never be parented to the gendisk. > > The odd part is that the scsi_device holds a reference to the queue. > That creates a reference loop: > > scsi_device holds ref torequest_queue (done explicitly) > request_queue holds ref togendisk (implicit, parent-child) agreed. > gendisk holds ref toscsi_device (implicit, parent-child) Where exactly does this last part happen? the scsi_device is a mid layer object; the mid-layer doesn't know about gendisks. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
drivers/scsi/* compile warnings
Hi , on current git ( a3634d7169f56eca5e349fce2f1de228fc10efda ) I see the following compile warning from some scsi drivers. Someone may want to look at these :) ... --($:/work/crazy/linux-git/fresh-linux2.6)-- grep ^[a-z] build.log|grep scsi drivers/scsi/advansys.c:71:2: warning: #warning this driver is still not properly converted to the DMA API drivers/scsi/advansys.c: In function 'advansys_board_found': drivers/scsi/advansys.c:13874: warning: label 'err_shost' defined but not used drivers/scsi/advansys.c: At top level: drivers/scsi/advansys.c:11894: warning: 'Default_3550_EEPROM_Config' defined but not used drivers/scsi/advansys.c:11932: warning: 'ADVEEP_3550_Config_Field_IsChar' defined but not used drivers/scsi/advansys.c:11970: warning: 'Default_38C0800_EEPROM_Config' defined but not used drivers/scsi/advansys.c:12035: warning: 'ADVEEP_38C0800_Config_Field_IsChar' defined but not used drivers/scsi/advansys.c:12100: warning: 'Default_38C1600_EEPROM_Config' defined but not used drivers/scsi/advansys.c:12165: warning: 'ADVEEP_38C1600_Config_Field_IsChar' defined but not used drivers/scsi/advansys.c: In function 'advansys_board_found': drivers/scsi/advansys.c:13400: warning: 'share_irq' may be used uninitialized in this function drivers/scsi/advansys.c:13400: warning: 'ret' may be used uninitialized in this function drivers/scsi/ultrastor.c: In function 'find_and_clear_bit_16': drivers/scsi/ultrastor.c:303: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:302: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c: In function 'ultrastor_host_reset': drivers/scsi/ultrastor.c:1013: warning: passing argument 1 of '__constant_c_and_count_memset' discards qualifiers from pointer target type drivers/scsi/ultrastor.c: At top level: drivers/scsi/ultrastor.c:1202: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:1202: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c: In function 'ultrastor_queuecommand': drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:698: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:302: warning: matching constraint does not allow a register drivers/scsi/ultrastor.c:302: warning: matching constraint does not allow a register drivers/scsi/aic7xxx_old.c:8902: warning: 'aic7xxx_configure_bugs' defined but not used drivers/scsi/aic7xxx_old.c: In function 'aic7xxx_free': drivers/scsi/aic7xxx_old.c:8490: warning: passing argument 3 of 'pci_free_consistent' discards qualifiers from pointer target type drivers/scsi/sym53c416.c: In function 'sym53c416_detect': drivers/scsi/sym53c416.c:638: warning: the address of 'sym53c416' will always evaluate as 'true' drivers/scsi/sym53c416.c:644: warning: the address of 'sym53c416_1' will always evaluate as 'true' drivers/scsi/sym53c416.c:650: warning: the address of 'sym53c416_2' will always evaluate as 'true' drivers/scsi/sym53c416.c:656: warning: the address of 'sym53c416_3' will always evaluate as 'true' ... sh scripts/ver_linux If some fields are empty or look unusual you may have an old version. Compare to the current minimal requirements in Documentation/Changes. Linux lara 2.6.24-rc1-g97855b49 #355 SMP Tue Oct 30 18:42:16 CET 2007 i686 Intel(R) Xeon(TM) CPU 2.00GHz GenuineIntel GNU/Linux Gnu C 4.2.2 Gnu make 3.81 binutils 2.18.50.0.2.20071001 util-linux 2.13 mount 2.13 module-init-tools 3.2.2 e2fsprogs 1.40.2 xfsprogs 2.9.4 pcmciautils014 quota-tools3.15. Linux C Library2.7 Dynamic linker (ldd) 2.7 Linux C++ Library 6.0.9 Procps 3.2.7 Net-tools 1.60 Kbd1.12 Sh-utils 6.9 udev 116 Modules Loaded fuse pc87360 hwmon_vid eeprom adm1021 sr_mod shpchp pci_hotplug iTCO_wdt iTCO_vendor_support intel_agp i82860_edac i2c_i801 edac_core cdrom agpgart 3c59x mii ext4dev jbd2 crc16 loop lp parport_pc parport evdev config is a allmodconfig with CONFIG_PCI and CONFIG_PM disabled. Regards, Gabriel - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 31 Oct 2007, James Bottomley wrote: > > Yes, the queue is a child of the disk. > > Right, so this goes gendisk->queue (-> meaning parent of, or takes > reference to) No, no! The _child_ takes an implicit reference to the _parent_, not the other way around. > > > The scsi_device has a ref to the queue > > > > Yeah, while the queue is a grandchild of the scsi_device with the > > unified sysfs layout. > > No, the scsi_device is a direct parent of the queue, so we have > > scsi_device->queue Wrong -- the gendisk is the direct parent of the queue. The relevant line is in ll_rw_blk.c:blk_register_queue(): q->kobj.parent = kobject_get(&disk->dev.kobj); > > Yes, sounds right. We need to break that deleted-but-wait-for-cleanup at > > least at one of the devices involved. > > But it's broken when the driver is unbound. Diagrammatically it's: > > scsi_disk -> scsi_device -> queue > -> gendisk -> > > It's not circular, it's released when scsi_disk is released. It can > become circular if there's some hidden dependency between any of the > components ... but I don't think there is. Forget about the scsi_disk. It isn't part of the problem. Just concentrate on the scsi_device, the gendisk, and the queue. We have: scsi_device <- gendisk <- queue <- scsi_device, where "A <- B" means that B holds a reference to A. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 31 Oct 2007, James Bottomley wrote: > On Wed, 2007-10-31 at 07:32 -0700, Greg KH wrote: > > Hm, I seem to have missed the part in this thread where someone said > > that it was valid to have a parent reference a child device. That's > > just wrong and needs to be fixed. Is that in the scsi layer somewhere? > > The block layer? It sure isn't in the driver core... > > This is the piece I'm still not clear on. It's something to do with the > gendisk. I'd have to look in block, but I believe the queue takes a ref > to the gendisk. > > The scsi_device has a ref to the queue and the scsi_disk (in sd) has a > ref to both the scsi_device and the gendisk. That means, until sd is > unbound and the scsi_disk released, there's an implied unbreakable > reference chain. > > at least, I think that's what the problem is. No, you haven't got it right. Parent Child Grandchild -- - -- scsi_device gendisk request_queue The odd part is that the scsi_device holds a reference to the queue. That creates a reference loop: scsi_device holds ref torequest_queue (done explicitly) request_queue holds ref togendisk (implicit, parent-child) gendisk holds ref toscsi_device (implicit, parent-child) The scsi_disk adds confusion to the picture but it doesn't make things any worse than they already are. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Tue, 30 Oct 2007, Greg KH wrote: > > Since the SCSI stack is now in much better shape, there doesn't seem to > > be any reason to keep the old code. Do you agree that the patch below > > is worth merging? I submitted it to Greg some time ago, but he didn't > > want to accept it without some assurance that it is now safe. > > > > It would fix the problem Kay and saw with the circular references, > > because the references would all get dropped when the scsi_device is > > removed instead of waiting for a release that will never come. > > Hm, will this really solve the problem properly? I kept that above > patch because of the very real problem where a device in the middle of > the device tree can go away without the lower devices being gone. When you say "go away", do you mean "be deleted (deallocated)"? In general that should not pose a problem. Once a child has been unregistered it should not try to access its parent's structure any more. Hence, if the parent structure gets deallocated before the child structure, nothing bad will happen. Or when you say "go away", do you mean "be removed (unregistered)"? If a parent is removed while it still has children registered, that's a genuine bug. (In fact, driver_del() should check for this and issue a WARN_ON if it ever happens.) The driver doing the remove needs to be fixed. > The example at the time was usb-serial devices. The physical USB device > goes away, but the tty device that userspace has open still sticks > around until userspace closes the char device. Now you are using "goes away" to mean "is unplugged from the computer"! :-) There's nothing wrong with the physical device being unplugged, so long as the logical usb_device structure remains allocated. And since the usb_device is the parent of the tty device (it is the parent, isn't it?), it _will_ remain pinned while the tty device is registered. So the only problem that could arise would be if the tty device and the usb_device are both unregistered while userspace continues to hold the char device file open. There are two possible ways to handle this: 1. Ideally, the usb-serial driver would realize that the USB device was disconnected (by checking a private flag) and would therefore avoid trying to access the usb_device structure. 2. If that's impractical, the usb-serial driver could take an explicit reference to the usb_device structure each time the char device file is opened, and drop the reference when the file is closed. Either approach would work. > With your patch, will things break for this usage model? Not if the usb-serial drivers are written properly. > This kobject cleanup path is so fickle, I hate having to touch it :( Is usb-serial the worst case? Then if it turns out to be compatible with this change, all other problems will be equally fixable. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 16:40 +0100, Kay Sievers wrote: > On Wed, 2007-10-31 at 10:15 -0500, James Bottomley wrote: > > On Wed, 2007-10-31 at 07:32 -0700, Greg KH wrote: > > > Hm, I seem to have missed the part in this thread where someone said > > > that it was valid to have a parent reference a child device. That's > > > just wrong and needs to be fixed. Is that in the scsi layer somewhere? > > > The block layer? It sure isn't in the driver core... > > > > This is the piece I'm still not clear on. It's something to do with the > > gendisk. I'd have to look in block, but I believe the queue takes a ref > > to the gendisk. > > Yes, the queue is a child of the disk. Right, so this goes gendisk->queue (-> meaning parent of, or takes reference to) > > The scsi_device has a ref to the queue > > Yeah, while the queue is a grandchild of the scsi_device with the > unified sysfs layout. No, the scsi_device is a direct parent of the queue, so we have scsi_device->queue > > and the scsi_disk (in sd) has a > > ref to both the scsi_device and the gendisk. That means, until sd is > > unbound and the scsi_disk released, there's an implied unbreakable > > reference chain. > > > > at least, I think that's what the problem is. > > Yes, sounds right. We need to break that deleted-but-wait-for-cleanup at > least at one of the devices involved. But it's broken when the driver is unbound. Diagrammatically it's: scsi_disk -> scsi_device -> queue -> gendisk -> It's not circular, it's released when scsi_disk is released. It can become circular if there's some hidden dependency between any of the components ... but I don't think there is. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 10:15 -0500, James Bottomley wrote: > On Wed, 2007-10-31 at 07:32 -0700, Greg KH wrote: > > Hm, I seem to have missed the part in this thread where someone said > > that it was valid to have a parent reference a child device. That's > > just wrong and needs to be fixed. Is that in the scsi layer somewhere? > > The block layer? It sure isn't in the driver core... > > This is the piece I'm still not clear on. It's something to do with the > gendisk. I'd have to look in block, but I believe the queue takes a ref > to the gendisk. Yes, the queue is a child of the disk. > The scsi_device has a ref to the queue Yeah, while the queue is a grandchild of the scsi_device with the unified sysfs layout. > and the scsi_disk (in sd) has a > ref to both the scsi_device and the gendisk. That means, until sd is > unbound and the scsi_disk released, there's an implied unbreakable > reference chain. > > at least, I think that's what the problem is. Yes, sounds right. We need to break that deleted-but-wait-for-cleanup at least at one of the devices involved. Thanks, Kay - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, 2007-10-31 at 07:32 -0700, Greg KH wrote: > Hm, I seem to have missed the part in this thread where someone said > that it was valid to have a parent reference a child device. That's > just wrong and needs to be fixed. Is that in the scsi layer somewhere? > The block layer? It sure isn't in the driver core... This is the piece I'm still not clear on. It's something to do with the gendisk. I'd have to look in block, but I believe the queue takes a ref to the gendisk. The scsi_device has a ref to the queue and the scsi_disk (in sd) has a ref to both the scsi_device and the gendisk. That means, until sd is unbound and the scsi_disk released, there's an implied unbreakable reference chain. at least, I think that's what the problem is. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Wed, Oct 31, 2007 at 11:46:30AM +0100, Kay Sievers wrote: > On Tue, 2007-10-30 at 21:25 -0700, Greg KH wrote: > > On Mon, Oct 29, 2007 at 08:13:17PM +0100, Kay Sievers wrote: > > > On Mon, 2007-10-29 at 14:47 -0400, Alan Stern wrote: > > > > On Mon, 29 Oct 2007, James Bottomley wrote: > > > > > > > > > I'm not sure if we can do this patch. If you kill a device, you see > > > > > there's a processing state in scsi_prep_fn() for SDEV_DEL, which has a > > > > > printk message I see quite often when I unplug devices while they're > > > > > operating. > > > > > > > > > > If you NULL out and free the request queue immediately after setting > > > > > SDEV_DEL, without allowing the devices and filesystems to finish, are > > > > > you sure we're not going to get a null deref on sdev->request_queue? > > > > > > > > Let's get at this another way. The patch below probably should be > > > > applied in any case. It's essentially a reversion of this patch from > > > > 2003: > > > > > > > > http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=10921a8f1305b8ec97794941db78b825db5839bc > > > > > > > > which was written to compensate for problems in the SCSI stack. The > > > > idea was to avoid releasing a kobject before all its children were > > > > released. However as far as I can see, in general we should be able to > > > > release a kobject as soon as all its children are removed and its > > > > refcount drops to 0, without waiting for the children to be released. > > > > To put it another way, once a child is removed from visibility it > > > > should not try (or need) to access its parent at all. If it has to > > > > then it should take an explicit reference to the parent. > > > > > > > > Since the SCSI stack is now in much better shape, there doesn't seem to > > > > be any reason to keep the old code. Do you agree that the patch below > > > > is worth merging? I submitted it to Greg some time ago, but he didn't > > > > want to accept it without some assurance that it is now safe. > > > > > > > > It would fix the problem Kay and saw with the circular references, > > > > because the references would all get dropped when the scsi_device is > > > > removed instead of waiting for a release that will never come. > > > > > > It indeed fixes my problem. And it sounds you are right, the "fix" from > > > 2003 is probably just a paper-over for a missing explicit reference that > > > time. > > > > > > I'm all for giving the reversion a try, and add explicit parent get/put > > > if needed somewhere. If, for some reason, that will not happen, I'll > > > need to do something like this in the block patch, which will then be a > > > "fix for the paper-over solution for an unknown bug". :) > > > > > > > > > --- a/fs/partitions/check.c > > > +++ b/fs/partitions/check.c > > > ... > > > + device_del(&disk->dev); > > > + > > > + /* > > > +* disconnect from parent device, so the parent can clean up > > > +* without waiting for us to clean up > > > +* > > > +* the driver core took this reference while we added ourself as > > > +* a child of the parent device > > > +*/ > > > + parent = disk->dev.kobj.parent; > > > > Save off the parent before calling device_del() otherwise it could be a > > stale pointer, right? > > I still hold a ref, the one I drop a few lines later. It should be safe. Ok, but it's still wrong that this is needed :) > > > + disk->dev.kobj.parent = NULL; > > > + disk->dev.parent = NULL; > > > + kobject_put(parent); > > > > No, that's just wrong, if this is needed, something else really is > > broken, and I don't think it's the driver core... > > It's just a kobject_orphan() function. We need to break the circiular > reference somewhere, one of the objects in the circle has to clean up, > to allow the others to clean up. The objects are properly deleted, but > all final references are only dropped on cleanup. The kobject_orphan() > here is just what Alan's patch is doing for the whole core. > > With the current logic, if you have any parent referencing a child, the > core will deadlock, and never reach any cleanup funtion, right? That's > the loop I need to break here. Hm, I seem to have missed the part in this thread where someone said that it was valid to have a parent reference a child device. That's just wrong and needs to be fixed. Is that in the scsi layer somewhere? The block layer? It sure isn't in the driver core... thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> OSame problem here, if grp->is_visible is not set, sysfs_add_file() would
> never be called, right?
>
> Other than the logic problem (I think), I have no issue with this idea
> at all. Care to redo this so it works?
It's a fair cop govenor ... new patch attached.
James
---
Index: BUILD-2.6/fs/sysfs/group.c
===
--- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.0 -0500
+++ BUILD-2.6/fs/sysfs/group.c 2007-10-31 09:29:14.0 -0500
@@ -16,25 +16,31 @@
#include "sysfs.h"
-static void remove_files(struct sysfs_dirent *dir_sd,
+static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
+ int i;
- for (attr = grp->attrs; *attr; attr++)
- sysfs_hash_and_remove(dir_sd, (*attr)->name);
+ for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+ if (!grp->is_visible ||
+ grp->is_visible(kobj, *attr, i))
+ sysfs_hash_and_remove(dir_sd, (*attr)->name);
}
-static int create_files(struct sysfs_dirent *dir_sd,
+static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int error = 0;
+ int error = 0, i;
- for (attr = grp->attrs; *attr && !error; attr++)
- error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
+ for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
+ if (!grp->is_visible ||
+ grp->is_visible(kobj, *attr, i))
+ error |=
+ sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
if (error)
- remove_files(dir_sd, grp);
+ remove_files(dir_sd, kobj, grp);
return error;
}
@@ -54,7 +60,7 @@ int sysfs_create_group(struct kobject *
} else
sd = kobj->sd;
sysfs_get(sd);
- error = create_files(sd, grp);
+ error = create_files(sd, kobj, grp);
if (error) {
if (grp->name)
sysfs_remove_subdir(sd);
@@ -75,7 +81,7 @@ void sysfs_remove_group(struct kobject *
} else
sd = sysfs_get(dir_sd);
- remove_files(sd, grp);
+ remove_files(sd, kobj, grp);
if (grp->name)
sysfs_remove_subdir(sd);
Index: BUILD-2.6/include/linux/sysfs.h
===
--- BUILD-2.6.orig/include/linux/sysfs.h2007-10-28 17:20:06.0
-0500
+++ BUILD-2.6/include/linux/sysfs.h 2007-10-30 13:24:06.0 -0500
@@ -32,6 +32,8 @@ struct attribute {
struct attribute_group {
const char *name;
+ int (*is_visible)(struct kobject *,
+ struct attribute *, int);
struct attribute**attrs;
};
Index: BUILD-2.6/kernel/params.c
===
--- BUILD-2.6.orig/kernel/params.c 2007-10-29 01:18:43.0 -0500
+++ BUILD-2.6/kernel/params.c 2007-10-29 01:18:49.0 -0500
@@ -472,7 +472,7 @@ param_sysfs_setup(struct module_kobject
sizeof(mp->grp.attrs[0]));
size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
- mp = kmalloc(size[0] + size[1], GFP_KERNEL);
+ mp = kzalloc(size[0] + size[1], GFP_KERNEL);
if (!mp)
return ERR_PTR(-ENOMEM);
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] megaraid: Dell CERC ATA100/4ch support revisited
Hi all,
I finally managed to track down the outstanding Del CERC issue.
Problem is the megaraid_mbox does not find any devices with
firmware levels >= 6.62. And telling users to downgrade their
BIOS is not always an option. So here's the problem:
Megaraid F/W implements a feature called 'random deletion'.
Appearently it's used for delete logical volumes on the fly
(don't ask me for details, I've no idea).
So, for handling this they shift the target IDs for the
logical channels by 0x80.
Only the legacy megaraid driver shifts the ID only for
I/O commands, whereas the megaraid_mbox driver shifts
the ID for _all_ commands.
This results in the INQUIRY command being sent to the
wrong channel, where it doesn't detect any devices.
Not surprisingly, really.
So as I don't have a clue which behavior is the correct
one we should take the safe route and just disable this
feature if an offending firmware is found.
Maybe Sumant can shed some light here. But at least we
now know the reason behind this incompability.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[EMAIL PROTECTED] +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
From: Hannes Reinecke <[EMAIL PROTECTED]>
Subject: Dell CERC support for megaraid_mbox
Newer Dell CERC firmware (>= 6.62) implement a random deletion
handling compatible with the legacy megaraid driver.
The legacy handling shifted the target ID by 0x80 only for I/O
commands (READ/WRITE/etc), whereas megaraid_mbox shifts the
target ID always if random deletion is supported.
The resulted in megaraid_mbox sending an INQUIRY to the wrong
channel, and not finding any devices, obviously.
So we disable the random deletion support if the offending
firmware is found.
Signed-off-by: Hannes Reinecke <[EMAIL PROTECTED]>
diff --git a/drivers/scsi/megaraid/megaraid_mbox.c
b/drivers/scsi/megaraid/megaraid_mbox.c
index c6a53dc..8c84049 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -3172,6 +3172,23 @@ megaraid_mbox_support_random_del(adapter_t *adapter)
uint8_t raw_mbox[sizeof(mbox_t)];
int rval;
+ /*
+* Newer firmware on Dell CERC expect a different
+* random deletion handling, so disable it.
+*/
+ if (adapter->pdev->vendor == PCI_VENDOR_ID_AMI &&
+ adapter->pdev->device == PCI_DEVICE_ID_AMI_MEGARAID3 &&
+ adapter->pdev->subsystem_vendor == PCI_VENDOR_ID_DELL &&
+ adapter->pdev->subsystem_device == PCI_SUBSYS_ID_CERC_ATA100_4CH &&
+ (adapter->fw_version[0] > '6' ||
+(adapter->fw_version[0] == '6' &&
+ adapter->fw_version[2] > '6') ||
+(adapter->fw_version[0] == '6'
+ && adapter->fw_version[2] == '6'
+ && adapter->fw_version[3] > '1'))) {
+ con_log(CL_DLEVEL1, ("megaraid: disable random deletion\n"));
+ return 0;
+ }
mbox = (mbox_t *)raw_mbox;
diff --git a/drivers/scsi/megaraid/megaraid_mbox.h
b/drivers/scsi/megaraid/megaraid_mbox.h
index 626459d..c1d86d9 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.h
+++ b/drivers/scsi/megaraid/megaraid_mbox.h
@@ -88,6 +88,7 @@
#define PCI_SUBSYS_ID_PERC3_QC 0x0471
#define PCI_SUBSYS_ID_PERC3_DC 0x0493
#define PCI_SUBSYS_ID_PERC3_SC 0x0475
+#define PCI_SUBSYS_ID_CERC_ATA100_4CH 0x0511
#define MBOX_MAX_SCSI_CMDS 128 // number of cmds reserved for kernel
RE: [PATCH] aacraid: don't assign cpu_to_le32(constant) to u8
ACK Sincerely -- Mark Salyzyn > -Original Message- > From: Stephen Rothwell [mailto:[EMAIL PROTECTED] > Sent: Wednesday, October 31, 2007 12:02 AM > To: AACRAID > Cc: linux-scsi@vger.kernel.org; LKML > Subject: [PATCH] aacraid: don't assign cpu_to_le32(constant) to u8 > > Noticed on PowerPC allmod config build: > > drivers/scsi/aacraid/commsup.c:1342: warning: large integer > implicitly truncated to unsigned type > drivers/scsi/aacraid/commsup.c:1343: warning: large integer > implicitly truncated to unsigned type > drivers/scsi/aacraid/commsup.c:1344: warning: large integer > implicitly truncated to unsigned type > > Signed-off-by: Stephen Rothwell <[EMAIL PROTECTED]> > --- > drivers/scsi/aacraid/commsup.c |6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > -- > Cheers, > Stephen Rothwell[EMAIL PROTECTED] > > diff --git a/drivers/scsi/aacraid/commsup.c > b/drivers/scsi/aacraid/commsup.c > index 240a0bb..b9682a8 100644 > --- a/drivers/scsi/aacraid/commsup.c > +++ b/drivers/scsi/aacraid/commsup.c > @@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac) > aif = (struct aac_aifcmd *)hw_fib->data; > aif->command = cpu_to_le32(AifCmdEventNotify); > aif->seqnum = cpu_to_le32(0x); > - aif->data[0] = cpu_to_le32(AifEnExpEvent); > - aif->data[1] = cpu_to_le32(AifExeFirmwarePanic); > - aif->data[2] = cpu_to_le32(AifHighPriority); > + aif->data[0] = AifEnExpEvent; > + aif->data[1] = AifExeFirmwarePanic; > + aif->data[2] = AifHighPriority; > aif->data[3] = cpu_to_le32(BlinkLED); > > /* > -- > 1.5.3.4 > - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
On Wed, 31 Oct 2007 11:37:36 +0100, Stefan Richter <[EMAIL PROTECTED]> wrote: > > mask_out() would also imply that the common use case is to have all > > attributes in the group created and that you need to take action to > > have an attribute not created. > > Here you have a point. But James has a point too when he says: > | We basically want to show capability by which file is present. Currently, if you register an attribute group, all of the attributes will show up. I find it more intuitive to have to block some attributes explicitely if I want to have more control, but the original logic is fine with me as well, if most people prefer that :) > Anyway, /if/ the reverse logic is preferred, it shouldn't be called > "mask_out()" but rather "is_masked()" or "is_hidden()" or the like. Sure. is_masked() would be fine. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
Hi Kay:
* Kay Sievers <[EMAIL PROTECTED]> [2007-10-31 03:01:56 +0100]:
> On Oct 31, 2007 1:40 AM, Mark M. Hoffman <[EMAIL PROTECTED]> wrote:
> > * James Bottomley <[EMAIL PROTECTED]> [2007-10-30 13:25:43 -0500]:
> > > On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > > > James Bottomley wrote:
> > > > >> > struct attribute_group {
> > > > >> >const char *name;
> > > > >> > + int (*filter_show)(struct kobject *,
> > > > >> > int);
> > > >
> > > > > Actually, it returns a true/false value indicating whether the given
> > > > > attribute should be displayed.
> > > >
> > > > How about this:
> > > >
> > > > int (*is_visible)(...);
> > >
> > > OK, so is this latest revision acceptable to everyone?
> >
> > I've just been hacking around in this area a bit, for a completely different
> > reason: there are literally 1000's of attributes in drivers/hwmon/*.c that
> > really want to be const, but which cannot be due to the current API. I was
> > about to propose some patches that move in a different direction...
>
> That isn't related to "dynamic attributes", right?
Right, it's not. That phrase was coined by the previous maintainer, Jean, to
describe a specific mechanism which makes for smaller code in drivers/hwmon.
> > IMHO the fundamental problem is struct attribute_group itself. This
> > structure
> > is nothing but a convenience for packaging the arguments to
> > sysfs_create_group
> > and sysfs_remove_group.
>
> That "problem" is actually a good thing. If you look at the change
> rate of the internal kernel API, it saves us so much trouble. Like in
> this case, James can just add a callback without caring about any
> (almost :)) of the current users.
Given my patch, he could also just add sysfs_create_attr_group_filtered(...).
This would affect current users even less than what you suggest because it
wouldn't bloat the struct that they all use.
> > Those functions should take the contents of that
> > struct as direct arguments.
>
> I think we should move in the opposite direction. You are right, it
> isn't neccessarily pretty, but having encapsulations like this saves
> us a lot of trouble while interacting with so many other people and
> extending API's all the time. It's a trade, and it's a good one, if
> you need to maintain code that has so many callers, and so many
> architectures, you can't even check that you don't break them.
Again, I don't see how it is better to bloat a struct with many users instead
of just adding a function for the few that need the extra functionality. My
patch also requires 0 change for everyone else.
> > I haven't finished the patch series to implement
> > this, but since I noticed your patch I thought I'd better speak up now.
> > Here's
> > the first... the idea is to eventually deprecate
> > sysfs_[create|remove]_group()
> > altogether.
>
> Again, I don't think, that we want to get rid of the struct container
> housing all the parameters and beeing open for future extensions
> without changing all the callers.
BTW: I hope you're not resisting based on the prospect of deprecating those
two functions. I don't really have time to push such a huge patch either.
It wouldn't hurt just to keep them.
> > The current declaration of struct attribute_group prevents long lists of
> > attributes from being marked const. Ideally, the second argument to the
> > sysfs_create_group() and sysfs_remove_group() functions would be marked
> > "deep"
> > const, but C has no such construct. This patch provides a parallel set
> > of
> > functions with the desired decoration.
>
> What do we get out of this constification compared to the current code?
Conceptual correctness, for one, but also it allows hwmon drivers to move as
much as 1K each out of the data section and into text. This is useful for the
embedded XIP scenario.
And please don't underestimate conceptual correctness: at least hwmon (and
probably many other users) really *rely* on the core not to modify the contents
of struct attribute_group (specifically, the data to which its members point).
Without the proper const qualifiers, this is not externally guaranteed.
Regards,
--
Mark M. Hoffman
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)
On Tue, 2007-10-30 at 21:25 -0700, Greg KH wrote: > On Mon, Oct 29, 2007 at 08:13:17PM +0100, Kay Sievers wrote: > > On Mon, 2007-10-29 at 14:47 -0400, Alan Stern wrote: > > > On Mon, 29 Oct 2007, James Bottomley wrote: > > > > > > > I'm not sure if we can do this patch. If you kill a device, you see > > > > there's a processing state in scsi_prep_fn() for SDEV_DEL, which has a > > > > printk message I see quite often when I unplug devices while they're > > > > operating. > > > > > > > > If you NULL out and free the request queue immediately after setting > > > > SDEV_DEL, without allowing the devices and filesystems to finish, are > > > > you sure we're not going to get a null deref on sdev->request_queue? > > > > > > Let's get at this another way. The patch below probably should be > > > applied in any case. It's essentially a reversion of this patch from > > > 2003: > > > > > > http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=10921a8f1305b8ec97794941db78b825db5839bc > > > > > > which was written to compensate for problems in the SCSI stack. The > > > idea was to avoid releasing a kobject before all its children were > > > released. However as far as I can see, in general we should be able to > > > release a kobject as soon as all its children are removed and its > > > refcount drops to 0, without waiting for the children to be released. > > > To put it another way, once a child is removed from visibility it > > > should not try (or need) to access its parent at all. If it has to > > > then it should take an explicit reference to the parent. > > > > > > Since the SCSI stack is now in much better shape, there doesn't seem to > > > be any reason to keep the old code. Do you agree that the patch below > > > is worth merging? I submitted it to Greg some time ago, but he didn't > > > want to accept it without some assurance that it is now safe. > > > > > > It would fix the problem Kay and saw with the circular references, > > > because the references would all get dropped when the scsi_device is > > > removed instead of waiting for a release that will never come. > > > > It indeed fixes my problem. And it sounds you are right, the "fix" from > > 2003 is probably just a paper-over for a missing explicit reference that > > time. > > > > I'm all for giving the reversion a try, and add explicit parent get/put > > if needed somewhere. If, for some reason, that will not happen, I'll > > need to do something like this in the block patch, which will then be a > > "fix for the paper-over solution for an unknown bug". :) > > > > > > --- a/fs/partitions/check.c > > +++ b/fs/partitions/check.c > > ... > > + device_del(&disk->dev); > > + > > + /* > > +* disconnect from parent device, so the parent can clean up > > +* without waiting for us to clean up > > +* > > +* the driver core took this reference while we added ourself as > > +* a child of the parent device > > +*/ > > + parent = disk->dev.kobj.parent; > > Save off the parent before calling device_del() otherwise it could be a > stale pointer, right? I still hold a ref, the one I drop a few lines later. It should be safe. > > + disk->dev.kobj.parent = NULL; > > + disk->dev.parent = NULL; > > + kobject_put(parent); > > No, that's just wrong, if this is needed, something else really is > broken, and I don't think it's the driver core... It's just a kobject_orphan() function. We need to break the circiular reference somewhere, one of the objects in the circle has to clean up, to allow the others to clean up. The objects are properly deleted, but all final references are only dropped on cleanup. The kobject_orphan() here is just what Alan's patch is doing for the whole core. With the current logic, if you have any parent referencing a child, the core will deadlock, and never reach any cleanup funtion, right? That's the loop I need to break here. Thanks, Kay - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
Cornelia Huck wrote:
> On Wed, 31 Oct 2007 10:52:35 +0100,
> Stefan Richter <[EMAIL PROTECTED]> wrote:
>> if (!grp->is_visible ||
>> grp->is_visible(kobj, *attr, i))
>> add or remove();
>>
>
> Hm, I find that a bit harder to parse...
if (grp->is_visible == NULL ||
grp->is_visible(kobj, *attr, i))
add or remove();
However, how beautiful the implementation of static void remove_files()
and static int create_files() looks doesn't matter. What's important is
that
struct attribute_group {
const char *name;
+ int (*is_visible)(struct kobject *,
+ struct attribute *, int);
struct attribute**attrs;
};
makes sense to users --- because this is the API.
[BTW, like most of the existing driver core APIs, there are kerneldoc
comments missing here.]
> mask_out() would also imply that the common use case is to have all
> attributes in the group created and that you need to take action to
> have an attribute not created.
Here you have a point. But James has a point too when he says:
| We basically want to show capability by which file is present.
Anyway, /if/ the reverse logic is preferred, it shouldn't be called
"mask_out()" but rather "is_masked()" or "is_hidden()" or the like.
--
Stefan Richter
-=-=-=== =-=- =
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] zfcp: add some internal zfcp adapter statistics
From: Swen Schillig <[EMAIL PROTECTED]>
add some statistics provided by the zFCP adapter to the sysfs
The new zFCP adapter statistics provide a variety of information
about the virtual adapter (subchannel). In order to collect this information
the zFCP driver is extended on one side to query the adapter and
on the other side summarize certain values which can then be fetched on demand.
This information is made available via files(attributes) in the sysfs
filesystem.
The information provided by the new zFCP adapter statistics can be fetched
by reading from the following files in the sysfs filesystem
/sys/class/scsi_host/host/seconds_active
/sys/class/scsi_host/host/requests
/sys/class/scsi_host/host/megabytes
/sys/class/scsi_host/host/utilization
These are the statistics on a virtual adapter (subchannel) level.
In addition latency information is provided on a SCSI device level (LUN) which
can be found at the following location
/sys/class/scsi_device//device/cmd_latency
/sys/class/scsi_device//device/read_latency
/sys/class/scsi_device//device/write_latency
The information provided is raw and not modified or interpreted by any means.
No interpretation or modification of the values is done by the zFCP driver.
The individual values are summed up during normal operation of the virtual
adapter.
An overrun of the variables is neither detected nor treated. The conclusion is
that
the file has to be read twice to make a meaningful statement, because only the
differences of the values
between the two reads can be used.
The statistics make use of the SCSI mid-layer interface to provide its data to
the user.
In detail two hooks from the scsi_host_template are used to integrate
the zFCP statistics.
struct scsi_host_template {
...
.shost_attrs = zfcp_a_stats_attrs,
.sdev_attrs = zfcp_sysfs_sdev_attrs,
...
};
Signed-off-by: Swen Schillig <[EMAIL PROTECTED]>
Signed-off-by: Christof Schmitt <[EMAIL PROTECTED]>
---
drivers/s390/scsi/zfcp_def.h | 13 +++
drivers/s390/scsi/zfcp_fsf.c | 36 ++
drivers/s390/scsi/zfcp_fsf.h | 29 +++-
drivers/s390/scsi/zfcp_scsi.c | 148 ++
4 files changed, 223 insertions(+), 3 deletions(-)
Index: scsi-misc/drivers/s390/scsi/zfcp_def.h
===
--- scsi-misc.orig/drivers/s390/scsi/zfcp_def.h
+++ scsi-misc/drivers/s390/scsi/zfcp_def.h
@@ -868,6 +868,17 @@ struct zfcp_erp_action {
struct timer_list timer;
};
+struct latency_cont {
+ u32 channel;
+ u32 fabric;
+ u32 counter;
+};
+
+struct zfcp_latencies {
+ struct latency_cont read;
+ struct latency_cont write;
+ struct latency_cont cmd;
+};
struct zfcp_adapter {
struct list_headlist; /* list of adapters */
@@ -883,6 +894,7 @@ struct zfcp_adapter {
u32 adapter_features; /* FCP channel features */
u32 connection_features; /* host connection
features */
u32hardware_version; /* of FCP channel */
+ u16 timer_ticks; /* time int for a tick */
struct Scsi_Host*scsi_host;/* Pointer to mid-layer */
struct list_headport_list_head;/* remote port list */
struct list_headport_remove_lh;/* head of ports to be
@@ -986,6 +998,7 @@ struct zfcp_unit {
all scsi_scan_target
requests have been
completed. */
+ struct zfcp_latencies latencies;
};
/* FSF request */
Index: scsi-misc/drivers/s390/scsi/zfcp_fsf.c
===
--- scsi-misc.orig/drivers/s390/scsi/zfcp_fsf.c
+++ scsi-misc/drivers/s390/scsi/zfcp_fsf.c
@@ -2079,6 +2079,7 @@ zfcp_fsf_exchange_config_evaluate(struct
fc_host_supported_classes(shost) =
FC_COS_CLASS2 | FC_COS_CLASS3;
adapter->hydra_version = bottom->adapter_type;
+ adapter->timer_ticks = bottom->timer_interval;
if (fc_host_permanent_port_name(shost) == -1)
fc_host_permanent_port_name(shost) =
fc_host_port_name(shost);
@@ -3823,6 +3824,33 @@ zfcp_fsf_send_fcp_command_task_managemen
return fsf_req;
}
+static void zfcp_fsf_req_latency(struct zfcp_fsf_req *fsf_req)
+{
+ struct fsf_qual_latency_info *lat_inf;
+ struct zfcp_unit *unit;
+
+ lat_inf = &fsf_req->qtcb->prefix.prot_status_qual.latency_info;
+ unit = fsf_req->unit;
+
+ switch (fsf_req->qtcb->bottom.io.data_direction) {
+ case FSF_DATADIR_READ:
+ unit->latencies.read.channel += lat_inf->channel_lat;
+ uni
Re: [PATCH] sysfs: add filter function to groups
On Wed, 31 Oct 2007 10:52:35 +0100,
Stefan Richter <[EMAIL PROTECTED]> wrote:
> Cornelia Huck wrote:
> > Greg KH <[EMAIL PROTECTED]> wrote:
> >> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> >>> + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> >>> + if (grp->is_visible &&
> >>> + grp->is_visible(kobj, *attr, i))
> >>> + sysfs_hash_and_remove(dir_sd, (*attr)->name);
> >> Hm, doesn't this break for the zillions of attribute groups that do not
> >> have the is_visible function set?
> ...
> > Would it make more sense then to turn the meaning of the callback
> > around?
> >
> > for (...) {
> > if (grp->mask_out && grp->mask_out(kobj, *attr, i))
> > continue;
> > error |= sysfs_add_file(...);
> > }
>
> if (!grp->is_visible ||
> grp->is_visible(kobj, *attr, i))
> add or remove();
>
Hm, I find that a bit harder to parse...
mask_out() would also imply that the common use case is to have all
attributes in the group created and that you need to take action to
have an attribute not created.
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
Cornelia Huck wrote:
> Greg KH <[EMAIL PROTECTED]> wrote:
>> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
>>> + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
>>> + if (grp->is_visible &&
>>> + grp->is_visible(kobj, *attr, i))
>>> + sysfs_hash_and_remove(dir_sd, (*attr)->name);
>> Hm, doesn't this break for the zillions of attribute groups that do not
>> have the is_visible function set?
...
> Would it make more sense then to turn the meaning of the callback
> around?
>
> for (...) {
> if (grp->mask_out && grp->mask_out(kobj, *attr, i))
> continue;
> error |= sysfs_add_file(...);
> }
if (!grp->is_visible ||
grp->is_visible(kobj, *attr, i))
add or remove();
--
Stefan Richter
-=-=-=== =-=- =
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sysfs: add filter function to groups
On Tue, 30 Oct 2007 20:55:06 -0700,
Greg KH <[EMAIL PROTECTED]> wrote:
> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> > Index: BUILD-2.6/fs/sysfs/group.c
> > ===
> > --- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.0 -0500
> > +++ BUILD-2.6/fs/sysfs/group.c 2007-10-30 12:35:47.0 -0500
> > @@ -16,25 +16,31 @@
> > #include "sysfs.h"
> >
> >
> > -static void remove_files(struct sysfs_dirent *dir_sd,
> > +static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> > const struct attribute_group *grp)
> > {
> > struct attribute *const* attr;
> > + int i;
> >
> > - for (attr = grp->attrs; *attr; attr++)
> > - sysfs_hash_and_remove(dir_sd, (*attr)->name);
> > + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> > + if (grp->is_visible &&
> > + grp->is_visible(kobj, *attr, i))
> > + sysfs_hash_and_remove(dir_sd, (*attr)->name);
>
> Hm, doesn't this break for the zillions of attribute groups that do not
> have the is_visible function set?
>
> > -static int create_files(struct sysfs_dirent *dir_sd,
> > +static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> > const struct attribute_group *grp)
> > {
> > struct attribute *const* attr;
> > - int error = 0;
> > + int error = 0, i;
> >
> > - for (attr = grp->attrs; *attr && !error; attr++)
> > - error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
> > + for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
> > + if (grp->is_visible &&
> > + grp->is_visible(kobj, *attr, i))
> > + error |=
> > + sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
>
> Same problem here, if grp->is_visible is not set, sysfs_add_file() would
> never be called, right?
>
> Other than the logic problem (I think), I have no issue with this idea
> at all. Care to redo this so it works?
Would it make more sense then to turn the meaning of the callback
around?
for (...) {
if (grp->mask_out && grp->mask_out(kobj, *attr, i))
continue;
error |= sysfs_add_file(...);
}
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
