Re: [flashrom] [PATCH] Automatically release I/O permissions on shutdown

[Carl-Daniel Hailfinger]

Am 21.07.2012 16:19 schrieb Michael Karcher:
> Am Mittwoch, den 18.07.2012, 01:48 +0200 schrieb Carl-Daniel Hailfinger:
> > get_io_perms() is renamed to rget_io_perms() and automatically registers
> > a function to release I/O permissions on shutdown.
>
> Sounds like a good idea, but what happens if two threads use different
> programmers, both needing I/O permissions? You would need atomic
> reference counts on the io permissions to fix that. I don't claim the
> current code would not fail in spectecular ways on that use, but AFAIK
> this is something we want to achieve in the long run (see the flashctx
> stuff for example).

I expect the current code to explode spectacularly if two or more
flashrom/libflashrom instances are active at the same time, regardless
of whether they are separate processes or just separate threads.
External programmers with their own timing (i.e. not the busy loop we
use right now) may even work with one (lib)flashrom process per device,
but there is a lot of work to do before we can even think of
multithreaded use of libflashrom, and we have to check all external
libraries (libusb etc) to find out if they are thread-safe as well.
> Apart from that, it seems good.
>
> Do we want to use the chance and replace the exit(1) in get_io_perms by
> a return value?

Yes, will do that.
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Michael Karcher

Thanks for the review.

New version.

get_io_perms() is renamed to rget_io_perms() and automatically registers
a function to release I/O permissions on shutdown.

Actually release I/O permissions on Solaris and iopl()-supporting
operating systems like Linux.

This patch fixes quite a few programmers which forgot to release I/O
permissions on shutdown, and it simplifies the shutdown and error
handling code for all others.

Do not call exit(1) if I/O permissions are denied and return an error
instead. Patch by Niklas Söderlund.

Signed-off-by: Carl-Daniel Hailfinger 
Signed-off-by: Niklas Söderlund 
Acked-by: Michael Karcher

Index: flashrom-auto_release_io_perms/ogp_spi.c
===================================================================
--- flashrom-auto_release_io_perms/ogp_spi.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/ogp_spi.c	(Arbeitskopie)
@@ -99,7 +99,6 @@
 {
    physunmap(ogp_spibar, 4096);
    pci_cleanup(pacc);
-	release_io_perms();

return 0;
 }
@@ -129,7 +128,8 @@
    	return 1;
    }

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi);

Index: flashrom-auto_release_io_perms/hwaccess.c
===================================================================
--- flashrom-auto_release_io_perms/hwaccess.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/hwaccess.c	(Arbeitskopie)
@@ -45,11 +45,27 @@
 int io_fd;
 #endif

-void get_io_perms(void)
+int release_io_perms(void *p)
 {
 #if defined(__DJGPP__) || defined(__LIBPAYLOAD__)
+#else
+#if defined (__sun) && (defined(__i386) || defined(__amd64))
+	sysi86(SI86V86, V86SC_IOPL, 0);
+#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined (__DragonFly__)
+	close(io_fd);
+#else 
+	iopl(0);
+#endif
+#endif
+	return 0;
+}
+
+/* Get I/O permissions with automatic permission release on shutdown. */
+int rget_io_perms(void)
+{
+#if defined(__DJGPP__) || defined(__LIBPAYLOAD__)
    /* We have full permissions by default. */
-	return;
+	return 0;
 #else
 #if defined (__sun) && (defined(__i386) || defined(__amd64))
    if (sysi86(SI86V86, V86SC_IOPL, PS_IOPL) != 0) {
@@ -65,18 +81,14 @@
    		   "and reboot, or reboot into \n");
    	msg_perr("single user mode.\n");
 #endif
-		exit(1);
+		return 1;
+	} else {
+		register_shutdown(release_io_perms, NULL);
    }
+	return 0;
 #endif
 }

-void release_io_perms(void)
-{
-#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__)
-	close(io_fd);
-#endif
-}
-
 #elif defined(__powerpc__) || defined(__powerpc64__) || defined(__ppc__) || defined(__ppc64__)

static inline void sync_primitive(void)
@@ -89,15 +101,11 @@
 }

/* PCI port I/O is not yet implemented on PowerPC. */
-void get_io_perms(void)
+int rget_io_perms(void)
 {
+	return 0;
 }

-/* PCI port I/O is not yet implemented on PowerPC. */
-void release_io_perms(void)
-{
-}
-
 #elif defined (__mips) || defined (__mips__) || defined (_mips) || defined (mips)

/* sync primitive is not needed because /dev/mem on MIPS uses uncached accesses
@@ -108,29 +116,22 @@
 }

/* PCI port I/O is not yet implemented on MIPS. */
-void get_io_perms(void)
+int rget_io_perms(void)
 {
+	return 0;
 }

-/* PCI port I/O is not yet implemented on MIPS. */
-void release_io_perms(void)
-{
-}
-
 #elif defined (__arm__)

static inline void sync_primitive(void)
 {
 }

-void get_io_perms(void)
+int rget_io_perms(void)
 {
+	return 0;
 }

-void release_io_perms(void)
-{
-}
-
 #else

#error Unknown architecture
Index: flashrom-auto_release_io_perms/drkaiser.c
===================================================================
--- flashrom-auto_release_io_perms/drkaiser.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/drkaiser.c	(Arbeitskopie)
@@ -60,7 +60,6 @@
    physunmap(drkaiser_bar, DRKAISER_MEMMAP_SIZE);
    /* Flash write is disabled automatically by PCI restore. */
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 };

@@ -68,7 +67,8 @@
 {
    uint32_t addr;

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

addr = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev);

Index: flashrom-auto_release_io_perms/gfxnvidia.c
===================================================================
--- flashrom-auto_release_io_perms/gfxnvidia.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/gfxnvidia.c	(Arbeitskopie)
@@ -84,7 +84,6 @@
     * by PCI restore.
     */
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

@@ -92,7 +91,8 @@
 {
    uint32_t reg32;

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia);

Index: flashrom-auto_release_io_perms/nicrealtek.c
===================================================================
--- flashrom-auto_release_io_perms/nicrealtek.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/nicrealtek.c	(Arbeitskopie)
@@ -56,13 +56,13 @@
 {
    /* FIXME: We forgot to disable software access again. */
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

int nicrealtek_init(void)
 {
-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek);

Index: flashrom-auto_release_io_perms/satamv.c
===================================================================
--- flashrom-auto_release_io_perms/satamv.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/satamv.c	(Arbeitskopie)
@@ -61,7 +61,6 @@
 {
    physunmap(mv_bar, 0x20000);
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

@@ -86,7 +85,8 @@
    uintptr_t addr;
    uint32_t tmp;

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

/* BAR0 has all internal registers memory mapped. */
    /* No need to check for errors, pcidev_init() will not return in case
@@ -162,7 +162,6 @@

error_out:
    pci_cleanup(pacc);
-	release_io_perms();
    return 1;
 }

Index: flashrom-auto_release_io_perms/internal.c
===================================================================
--- flashrom-auto_release_io_perms/internal.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/internal.c	(Arbeitskopie)
@@ -159,7 +159,6 @@

static int internal_shutdown(void *data)
 {
-	release_io_perms();
    return 0;
 }

@@ -226,7 +225,8 @@
    }
    free(arg);

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;
    if (register_shutdown(internal_shutdown, NULL))
    	return 1;

Index: flashrom-auto_release_io_perms/nicintel_spi.c
===================================================================
--- flashrom-auto_release_io_perms/nicintel_spi.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/nicintel_spi.c	(Arbeitskopie)
@@ -159,7 +159,6 @@

physunmap(nicintel_spibar, 4096);
    pci_cleanup(pacc);
-	release_io_perms();

return 0;
 }
@@ -168,7 +167,8 @@
 {
    uint32_t tmp;

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi);

Index: flashrom-auto_release_io_perms/nicnatsemi.c
===================================================================
--- flashrom-auto_release_io_perms/nicnatsemi.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/nicnatsemi.c	(Arbeitskopie)
@@ -54,13 +54,13 @@
 static int nicnatsemi_shutdown(void *data)
 {
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

int nicnatsemi_init(void)
 {
-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_natsemi);

Index: flashrom-auto_release_io_perms/rayer_spi.c
===================================================================
--- flashrom-auto_release_io_perms/rayer_spi.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/rayer_spi.c	(Arbeitskopie)
@@ -168,7 +168,8 @@
    	rayer_miso_bit = 4;
    }

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

/* Get the initial value before writing to any line. */
    lpt_outbyte = INB(lpt_iobase);
Index: flashrom-auto_release_io_perms/atahpt.c
===================================================================
--- flashrom-auto_release_io_perms/atahpt.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/atahpt.c	(Arbeitskopie)
@@ -60,7 +60,6 @@
 {
    /* Flash access is disabled automatically by PCI restore. */
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

@@ -68,7 +67,8 @@
 {
    uint32_t reg32;

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_4, ata_hpt);

Index: flashrom-auto_release_io_perms/nic3com.c
===================================================================
--- flashrom-auto_release_io_perms/nic3com.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/nic3com.c	(Arbeitskopie)
@@ -82,13 +82,13 @@
    }

pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

int nic3com_init(void)
 {
-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_3com);

Index: flashrom-auto_release_io_perms/satasii.c
===================================================================
--- flashrom-auto_release_io_perms/satasii.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/satasii.c	(Arbeitskopie)
@@ -62,7 +62,6 @@
 {
    physunmap(sii_bar, SATASII_MEMMAP_SIZE);
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

@@ -71,7 +70,8 @@
    uint32_t addr;
    uint16_t reg_offset;

-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

pcidev_init(PCI_BASE_ADDRESS_0, satas_sii);

Index: flashrom-auto_release_io_perms/nicintel.c
===================================================================
--- flashrom-auto_release_io_perms/nicintel.c	(Revision 1550)
+++ flashrom-auto_release_io_perms/nicintel.c	(Arbeitskopie)
@@ -64,7 +64,6 @@
    physunmap(nicintel_control_bar, NICINTEL_CONTROL_MEMMAP_SIZE);
    physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);
    pci_cleanup(pacc);
-	release_io_perms();
    return 0;
 }

@@ -73,9 +72,10 @@
    uintptr_t addr;

/* Needed only for PCI accesses on some platforms.
-	 * FIXME: Refactor that into get_mem_perms/get_io_perms/get_pci_perms?
+	 * FIXME: Refactor that into get_mem_perms/rget_io_perms/get_pci_perms?
     */
-	get_io_perms();
+	if (rget_io_perms())
+		return 1;

/* No need to check for errors, pcidev_init() will not return in case
     * of errors.
@@ -118,7 +118,6 @@
    physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);
 error_out:
    pci_cleanup(pacc);
-	release_io_perms();
    return 1;
 }

Index: flashrom-auto_release_io_perms/programmer.h
===================================================================
--- flashrom-auto_release_io_perms/programmer.h	(Revision 1550)
+++ flashrom-auto_release_io_perms/programmer.h	(Arbeitskopie)
@@ -302,8 +302,7 @@
 struct pci_dev *pci_card_find(uint16_t vendor, uint16_t device,
    		      uint16_t card_vendor, uint16_t card_device);
 #endif
-void get_io_perms(void);
-void release_io_perms(void);
+int rget_io_perms(void);
 #if CONFIG_INTERNAL == 1
 extern int is_laptop;
 extern int laptop_ok;