From: Colin Ngam <cngam@sgi.com> To: Jesse Barnes <jbarnes@engr.sgi.com>, Grant Grundler <iod00d@hp.com> Cc: Patrick Gefre <pfg@sgi.com>, "Luck, Tony" <tony.luck@intel.com>, Matthew Wilcox <matthew@wil.cx>, linux-kernel@vger.kernel.org, linux-ia64@vger.kernel.org Subject: Re: [PATCH] 2.6 SGI Altix I/O code reorganization Date: Wed, 06 Oct 2004 15:21:37 -0500 [thread overview] Message-ID: <416453CE.2516D2BC@sgi.com> (raw) In-Reply-To: 200410061327.28572.jbarnes@engr.sgi.com Jesse Barnes wrote: Hi Jesse/Grant, May be my response to Grant got lost .. anyway, here it is again. > On Wednesday, October 6, 2004 12:54 pm, Grant Grundler wrote: > > Colin, > > thanks for ACKing the feedback. > > I think there is still some confusion... > > > > On Wed, Oct 06, 2004 at 02:09:54PM -0500, Colin Ngam wrote: > > ... > > > > > > Mathew explained replacing the raw_pci_ops pointer is the Right Thing > > > > and I suspect it's easier to properly implement. > > > > > > I believe we did just that. We did not touch pci_root_ops. > > > > Correct. The patch ignores/overides pci_root_ops with sn_pci_root_ops > > (which is what I originally suggested). > > > > Mathew's point was only raw_pci_ops needs to point at a different > > set of struct pci_raw_ops (see include/linux/pci.h). > > Though now what's there seems awfully redundant, wouldn't you say? Just > allowing direct access to pci_root_ops is a much simpler approach and gets > rid of a bunch of extra, unneeded code (i.e. closer to Pat's original > version). The original mod, we took out the static from pci_root_ops() so that we can use it in io_init.c. We thought that would be the cleanest. We do not want to change pci_raw_ops(). It is doing exactly what we need, now that sn platform has the support for SAL pci reads and writes support. > > > > > Yes, would anybody allow us to make a platform specific callout > > > from within generic pcibios_fixup_bus()??? > > > > If it can be avoided, preferably not. But that's up to Jesse/Tony I think. > > If it was made a machine vector that's a no-op on everything but sn2, I think > it would be fine. Doing it for the general sn_pci_init routine would let us > get rid of the check for ia64_platform_is("sn2") in one of the routines, I > think (which is nice if only for the consistency). > > > Can you quote the bit of the patch which implements "if the bus does not > > exist" check? > > I can't find it. > > In the current code it's: > > for (i = 0; i < PCI_BUSES_TO_SCAN; i++) > if (pci_bus_to_vertex(i)) > pci_scan_bus(i, &sn_pci_ops, controller); > > which causes the next loop to only fixup existing busses. But I don't see it > in the new code. Probably not clear to all: +/* + * sn_pci_fixup_bus() - This routine sets up a bus's resources + * consistent with the Linux PCI abstraction layer. + */ +static void sn_pci_fixup_bus(int segment, int busnum) +{ + int status = 0; + int nasid, cnode; + struct pci_bus *bus; + struct pci_controller *controller; + struct pcibus_bussoft *prom_bussoft_ptr; + struct hubdev_info *hubdev_info; + void *provider_soft; + + status = + sal_get_pcibus_info((u64) segment, (u64) busnum, + (u64) ia64_tpa(&prom_bussoft_ptr)); + if (status > 0) { + return; /* bus # does not exist */ + } + + prom_bussoft_ptr = __va(prom_bussoft_ptr); + controller = sn_alloc_pci_sysdata(); + if (!controller) { + BUG(); + } + + bus = pci_scan_bus(busnum, &sn_pci_root_ops, controller); + if (bus == NULL) { + return; /* error, or bus already scanned */ + } The sal_get_pcibus_info() will fail if we do not find that bus number. If it fails, we do not call pci_scan_bus() Thanks. colin > > > > > One favour. Would you agree to letting this patch be included by Tony > > > and we will come up with another patch to fix the 2 obvious items listed > > > above? It will be great to avoid spinning this big patch. > > The patch is ok with me, I think it's a big improvement over what's there in > terms of readability. > > I just checked out sn_set_affinity_irq() and it's a bit hard to see what's > going on. Why does a new interrupt have to be allocated? Also, it looks > like the kfree() is one line too high, if sn_intr_alloc fails, we'll leak > new_sn_irq_info. > > Jesse
WARNING: multiple messages have this Message-ID (diff)
From: Colin Ngam <cngam@sgi.com> To: Jesse Barnes <jbarnes@engr.sgi.com>, Grant Grundler <iod00d@hp.com> Cc: Patrick Gefre <pfg@sgi.com>, "Luck, Tony" <tony.luck@intel.com>, Matthew Wilcox <matthew@wil.cx>, linux-kernel@vger.kernel.org, linux-ia64@vger.kernel.org Subject: Re: [PATCH] 2.6 SGI Altix I/O code reorganization Date: Wed, 06 Oct 2004 20:21:37 +0000 [thread overview] Message-ID: <416453CE.2516D2BC@sgi.com> (raw) In-Reply-To: 200410061327.28572.jbarnes@engr.sgi.com Jesse Barnes wrote: Hi Jesse/Grant, May be my response to Grant got lost .. anyway, here it is again. > On Wednesday, October 6, 2004 12:54 pm, Grant Grundler wrote: > > Colin, > > thanks for ACKing the feedback. > > I think there is still some confusion... > > > > On Wed, Oct 06, 2004 at 02:09:54PM -0500, Colin Ngam wrote: > > ... > > > > > > Mathew explained replacing the raw_pci_ops pointer is the Right Thing > > > > and I suspect it's easier to properly implement. > > > > > > I believe we did just that. We did not touch pci_root_ops. > > > > Correct. The patch ignores/overides pci_root_ops with sn_pci_root_ops > > (which is what I originally suggested). > > > > Mathew's point was only raw_pci_ops needs to point at a different > > set of struct pci_raw_ops (see include/linux/pci.h). > > Though now what's there seems awfully redundant, wouldn't you say? Just > allowing direct access to pci_root_ops is a much simpler approach and gets > rid of a bunch of extra, unneeded code (i.e. closer to Pat's original > version). The original mod, we took out the static from pci_root_ops() so that we can use it in io_init.c. We thought that would be the cleanest. We do not want to change pci_raw_ops(). It is doing exactly what we need, now that sn platform has the support for SAL pci reads and writes support. > > > > > Yes, would anybody allow us to make a platform specific callout > > > from within generic pcibios_fixup_bus()??? > > > > If it can be avoided, preferably not. But that's up to Jesse/Tony I think. > > If it was made a machine vector that's a no-op on everything but sn2, I think > it would be fine. Doing it for the general sn_pci_init routine would let us > get rid of the check for ia64_platform_is("sn2") in one of the routines, I > think (which is nice if only for the consistency). > > > Can you quote the bit of the patch which implements "if the bus does not > > exist" check? > > I can't find it. > > In the current code it's: > > for (i = 0; i < PCI_BUSES_TO_SCAN; i++) > if (pci_bus_to_vertex(i)) > pci_scan_bus(i, &sn_pci_ops, controller); > > which causes the next loop to only fixup existing busses. But I don't see it > in the new code. Probably not clear to all: +/* + * sn_pci_fixup_bus() - This routine sets up a bus's resources + * consistent with the Linux PCI abstraction layer. + */ +static void sn_pci_fixup_bus(int segment, int busnum) +{ + int status = 0; + int nasid, cnode; + struct pci_bus *bus; + struct pci_controller *controller; + struct pcibus_bussoft *prom_bussoft_ptr; + struct hubdev_info *hubdev_info; + void *provider_soft; + + status + sal_get_pcibus_info((u64) segment, (u64) busnum, + (u64) ia64_tpa(&prom_bussoft_ptr)); + if (status > 0) { + return; /* bus # does not exist */ + } + + prom_bussoft_ptr = __va(prom_bussoft_ptr); + controller = sn_alloc_pci_sysdata(); + if (!controller) { + BUG(); + } + + bus = pci_scan_bus(busnum, &sn_pci_root_ops, controller); + if (bus = NULL) { + return; /* error, or bus already scanned */ + } The sal_get_pcibus_info() will fail if we do not find that bus number. If it fails, we do not call pci_scan_bus() Thanks. colin > > > > > One favour. Would you agree to letting this patch be included by Tony > > > and we will come up with another patch to fix the 2 obvious items listed > > > above? It will be great to avoid spinning this big patch. > > The patch is ok with me, I think it's a big improvement over what's there in > terms of readability. > > I just checked out sn_set_affinity_irq() and it's a bit hard to see what's > going on. Why does a new interrupt have to be allocated? Also, it looks > like the kfree() is one line too high, if sn_intr_alloc fails, we'll leak > new_sn_irq_info. > > Jesse
next prev parent reply other threads:[~2004-10-06 20:41 UTC|newest] Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top 2004-10-05 20:34 [PATCH] 2.6 SGI Altix I/O code reorganization Luck, Tony 2004-10-05 20:34 ` Luck, Tony 2004-10-06 15:32 ` Patrick Gefre 2004-10-06 15:32 ` Patrick Gefre 2004-10-06 18:57 ` Grant Grundler 2004-10-06 18:57 ` Grant Grundler 2004-10-06 19:09 ` Colin Ngam 2004-10-06 19:09 ` Colin Ngam 2004-10-06 19:54 ` Grant Grundler 2004-10-06 19:54 ` Grant Grundler 2004-10-06 19:54 ` Colin Ngam 2004-10-06 19:54 ` Colin Ngam 2004-10-06 20:10 ` Patrick Gefre 2004-10-06 20:10 ` Patrick Gefre 2004-10-06 20:44 ` Jesse Barnes 2004-10-06 20:44 ` Jesse Barnes 2004-10-07 15:02 ` Patrick Gefre 2004-10-07 15:02 ` Patrick Gefre 2004-10-07 16:52 ` Jesse Barnes 2004-10-07 16:52 ` Jesse Barnes 2004-10-06 20:27 ` Jesse Barnes 2004-10-06 20:27 ` Jesse Barnes 2004-10-06 20:21 ` Colin Ngam [this message] 2004-10-06 20:21 ` Colin Ngam 2004-10-06 20:33 ` Matthew Wilcox 2004-10-06 20:33 ` Matthew Wilcox 2004-10-06 20:48 ` Grant Grundler 2004-10-06 20:48 ` Grant Grundler 2004-10-06 21:05 ` Matthew Wilcox 2004-10-06 21:05 ` Matthew Wilcox 2004-10-06 20:55 ` Colin Ngam 2004-10-06 20:55 ` Colin Ngam 2004-10-08 15:16 ` Colin Ngam 2004-10-08 15:16 ` Colin Ngam 2004-10-08 16:37 ` Jesse Barnes 2004-10-08 16:37 ` Jesse Barnes 2004-10-09 22:20 ` Grant Grundler 2004-10-09 22:20 ` Grant Grundler [not found] ` <4169A508.84FB19C7@sgi.com> 2004-10-11 14:03 ` Patrick Gefre 2004-10-11 14:03 ` Patrick Gefre 2004-10-08 22:37 ` Colin Ngam 2004-10-08 22:37 ` Colin Ngam -- strict thread matches above, loose matches on Subject: below -- 2004-10-11 20:49 Luck, Tony 2004-10-11 20:49 ` Luck, Tony 2004-10-07 17:06 Luck, Tony 2004-10-07 17:06 ` Luck, Tony 2004-10-07 17:22 ` Jesse Barnes 2004-10-07 17:22 ` Jesse Barnes 2004-10-07 18:59 ` Jes Sorensen 2004-10-07 18:59 ` Jes Sorensen 2004-10-05 19:16 Luck, Tony 2004-10-05 19:16 ` Luck, Tony 2004-10-05 19:35 ` Patrick Gefre 2004-10-05 19:35 ` Patrick Gefre 2004-10-05 5:13 Luck, Tony 2004-10-05 5:13 ` Luck, Tony 2004-10-05 15:43 ` Jesse Barnes 2004-10-05 15:43 ` Jesse Barnes 2004-10-05 16:22 ` Grant Grundler 2004-10-05 16:22 ` Grant Grundler 2004-10-05 17:45 ` Matthew Wilcox 2004-10-05 17:45 ` Matthew Wilcox 2004-10-05 19:00 ` Colin Ngam 2004-10-05 19:00 ` Colin Ngam 2004-10-05 19:10 ` Grant Grundler 2004-10-05 19:10 ` Grant Grundler 2004-10-05 19:15 ` Matthew Wilcox 2004-10-05 19:15 ` Matthew Wilcox 2004-10-05 18:20 ` Patrick Gefre 2004-10-05 18:20 ` Patrick Gefre 2004-10-05 18:34 ` Jesse Barnes 2004-10-05 18:34 ` Jesse Barnes 2004-10-04 21:57 Pat Gefre 2004-10-04 21:57 ` Pat Gefre 2004-10-05 15:48 ` Christoph Hellwig 2004-10-05 15:48 ` Christoph Hellwig 2004-10-05 18:26 ` Patrick Gefre 2004-10-05 18:26 ` Patrick Gefre 2004-10-05 23:30 ` Patrick Gefre 2004-10-05 23:30 ` Patrick Gefre 2004-10-05 15:50 ` Christoph Hellwig 2004-10-05 15:50 ` Christoph Hellwig
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=416453CE.2516D2BC@sgi.com \ --to=cngam@sgi.com \ --cc=iod00d@hp.com \ --cc=jbarnes@engr.sgi.com \ --cc=linux-ia64@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=matthew@wil.cx \ --cc=pfg@sgi.com \ --cc=tony.luck@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.