Bartlomiej Zolnierkiewicz wrote:
On Thursday 25 October 2007, Jeff Garzik wrote:Store our hwif indices at probe time, in order to eliminate a needless and ugly loop across all hwifs, searching for our pci device.It seems that we can simplify it even further and remove knowledge about hwifs altogether from suspend/resume methods.Signed-off-by: Jeff Garzik <[email protected]> --- drivers/ide/pci/sc1200.c | 76 +++++++++++++++++++++++++++++++--------------- 1 files changed, 51 insertions(+), 25 deletions(-) diff --git a/drivers/ide/pci/sc1200.c b/drivers/ide/pci/sc1200.c index d2c8b55..17e58d6 100644 --- a/drivers/ide/pci/sc1200.c +++ b/drivers/ide/pci/sc1200.c @@ -41,6 +41,8 @@ #define PCI_CLK_66 0x02 #define PCI_CLK_33A 0x03+#define SC1200_IFS 4+ static unsigned short sc1200_get_pci_clock (void) { unsigned char chip_id, silicon_revision; @@ -261,31 +263,32 @@ static void sc1200_set_pio_mode(ide_drive_t *drive, const u8 pio) }#ifdef CONFIG_PM-static ide_hwif_t *lookup_pci_dev (ide_hwif_t *prev, struct pci_dev *dev) -{ - int h; - - for (h = 0; h < MAX_HWIFS; h++) { - ide_hwif_t *hwif = &ide_hwifs[h]; - if (prev) { - if (hwif == prev) - prev = NULL; // found previous, now look for next match - } else { - if (hwif && hwif->pci_dev == dev) - return hwif; // found next match - } - } - return NULL; // not found -} - typedef struct sc1200_saved_state_s { __u32 regs[4]; } sc1200_saved_state_t;+static unsigned int pack_hwif_idx(u8 *idx)+{ + return (((unsigned int) idx[0]) << 0) | + (((unsigned int) idx[1]) << 8) | + (((unsigned int) idx[2]) << 16) | + (((unsigned int) idx[3]) << 24); +} + +static ide_hwif_t *sc1200_hwif(struct pci_dev *pdev, unsigned int iface) +{ + unsigned int packed_hwifs, idx; + + packed_hwifs = (unsigned long) pci_get_drvdata(pdev); + idx = (packed_hwifs >> (iface * 8)) & 0xff; + + return (idx == 0xff) ? NULL : &ide_hwifs[idx]; +}static int sc1200_suspend (struct pci_dev *dev, pm_message_t state){ - ide_hwif_t *hwif = NULL; + ide_hwif_t *hwif; + int i;printk("SC1200: suspend(%u)\n", state.event); @@ -295,9 +298,14 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state)// // Loop over all interfaces that are part of this PCI device: // - while ((hwif = lookup_pci_dev(hwif, dev)) != NULL) { + for (i = 0; i < SC1200_IFS; i++) { sc1200_saved_state_t *ss; unsigned int basereg, r; + + hwif = sc1200_hwif(dev, i); + if (!hwif) + continue; + // // allocate a permanent save area, if not already allocated // @@ -310,7 +318,7 @@ static int sc1200_suspend (struct pci_dev *dev, pm_message_t state) } ss = (sc1200_saved_state_t *)hwif->config_data; //- // Save timing registers: this may be unnecessary if + // Save timing registers: this may be unnecessary if// BIOS also does it // basereg = hwif->channel ? 0x50 : 0x40;Please take a close look at the line above and the next three lines: for (r = 0; r < 4; ++r) { pci_read_config_dword (hwif->pci_dev, basereg + (r<<2), &ss->regs[r]); } It is highly obfuscated but the sc1200_suspend() reads 16 bytes from the offset 0x40 (for the primary port) and puts them in the corresponding struct sc1200_saved_state_s buffer, then it reads another 16 bytes from the offset 0x50 (for the secondary port) and puts it in the another buffer. In summary sc1200_suspend() reads 32 continuous bytes from offset 0x40 and the exactly reverse operation happens in sc1200_resume(). Given that and the fact that struct sc1200_save_state_s buffers are used _only_ by sc1200_{suspend,resume}() we may safely convert the code to use one buffer for both ports (the whole PCI device). We just need to bump the size of struct sc1200_saved_state_s (from 4 to 8 double-words) and use pci_{get,set}_drvdata() instead of hwif->config_data. Then we can remove looping over interfaces completely from sc1200_{suspend,resume}()! :)
May I assume you'll handle that task? :) It sounds like you have a far better idea than mine, and my main goal -- fixing bugs and warning -- is accomplished anyway with the merging of patch #3.
Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
- Follow-Ups:
- Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop
- From: Bartlomiej Zolnierkiewicz <[email protected]>
- Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop
- References:
- [PATCH] sound/oss/pss: set_io_base() always returns success, mark it void
- From: Jeff Garzik <[email protected]>
- [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop
- From: Jeff Garzik <[email protected]>
- Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop
- From: Bartlomiej Zolnierkiewicz <[email protected]>
- [PATCH] sound/oss/pss: set_io_base() always returns success, mark it void
- Prev by Date: Re: [PATCH 2/2] CFS CGroup: Report usage
- Next by Date: Re: [PATCH 1/3 -v4] x86_64 EFI runtime service support: EFI basic runtime service support
- Previous by thread: Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop
- Next by thread: Re: [PATCH 2/3] drivers/ide/pci/sc1200.c: remove pointless hwif lookup loop
- Index(es):
 |