This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [flash_v2] Handle devices which don't initialise


>>>>> "Andrew" == Andrew Lunn <andrew@lunn.ch> writes:

    Andrew> I found a bug that stops my board from working.
    Andrew> Manufacturing can either put a Strata or an SST device on
    Andrew> the board and we want one image that runs both. So the
    Andrew> image has both drivers and we simple let one fail to
    Andrew> initialise. With my old code i only put devices that
    Andrew> initialised onto the head of the linked list. You changed
    Andrew> this so all devices in the table get put onto the list and
    Andrew> find_dev() checked the device was initialised before
    Andrew> returning it.

Yes, I see where I have changed the behaviour and broken things.

    Andrew> This however leads to one problem. After sorting the
    Andrew> linked list into order there is a check to see if any
    Andrew> devices overlap. This check failed because the device
    Andrew> which failed to initialise uses the same addresses as the
    Andrew> device which did initialise.

    Andrew> So i've changed the code to only add devices which
    Andrew> initialise onto the list and then optimised the code a
    Andrew> little.

Agree in principle, but:

    Andrew> Index: io/flash/current/src/flash.c
    Andrew> ===================================================================
    Andrew> RCS file: /cvs/ecos/ecos/packages/io/flash/current/src/flash.c,v
    Andrew> retrieving revision 1.26.2.14
    Andrew> diff -u -r1.26.2.14 flash.c
    Andrew> --- io/flash/current/src/flash.c	22 Nov 2004 13:25:23 -0000	1.26.2.14
    Andrew> +++ io/flash/current/src/flash.c	25 Nov 2004 13:39:43 -0000
    Andrew> @@ -154,15 +154,17 @@
    Andrew>    bool moved;
    Andrew>    struct cyg_flash_dev *dev, **previous_next;
 
    Andrew> -  // Place all devices on the list, unsorted for now.
    Andrew> +  // Place all devices that initialised on the list, unsorted for now.
    Andrew>    for (dev = &cyg_flashdevtab[0]; dev != &cyg_flashdevtab_end; dev++) {
    Andrew> -    dev->next  = flash_head;
    Andrew> -    flash_head = dev;
    Andrew> +    if (dev->init) {
    Andrew> +      dev->next  = flash_head;
    Andrew> +      flash_head = dev;
    Andrew> +    }
    Andrew>    }
   
    Andrew> -  // If there are no devices, abort. This should not happen because of
    Andrew> -  // the constraints on CYGHWR_IO_FLASH_DEVICE.
    Andrew> -  if (flash_head == NULL) {
    Andrew> +  // If there are no devices, abort. This could happen because none
    Andrew> +  // of the devices initialised. Also short cut for just one device.
    Andrew> +  if (flash_head == NULL || flash_head->next == NULL) {
    Andrew>      return false;
    Andrew>    }

This flash_head->next test looks wrong. If there are no valid flash
devices then initialization should fail. However if there is a single
flash device then initialization should succeed. In your scenario
flash_sort_and_check() will now return false, even though there is a
valid flash device, and hence the flash subsystem initialization as a
whole will fail and the init flag never gets set.

The following patch incorporates part of yours, so only initialized
devices get put on the list. I have also fixed cyg_flash_get_info() so
it will do the right thing for initialized/uninitialized flash
devices. I have removed the test for a single-entry list from
flash_sort_and_check(). In theory a shortcut is possible, returning
true if there is just one valid device. However the loops will still
work and finish immediately if there is just one device, so the
shortcut would only save a couple of cycles in a once-off init routine
yet add permanently to the code size.

Bart

Index: io/flash/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/flash/current/ChangeLog,v
retrieving revision 1.38.2.21
diff -u -r1.38.2.21 ChangeLog
--- io/flash/current/ChangeLog	22 Nov 2004 21:27:43 -0000	1.38.2.21
+++ io/flash/current/ChangeLog	28 Nov 2004 19:02:12 -0000
@@ -1,3 +1,17 @@
+2004-11-28  Bart Veer  <bartv@ecoscentric.com>
+
+	* src/flash.c (flash_sort_and_check): previous patch would have
+	resulted in init failure if only one device initialized.
+	(cyg_flash_get_info): handle per-device init flag
+
+2004-11-25  Andrew Lunn  <andrew.lunn@ascom.ch>
+
+	* src/flash.c (flash_sort_and_check): Don't add devices which
+	failed to initialise onto the list. Don't bother sorting the
+	list if its empty or only has one entry.
+	* src/flash.c (find_dev): All devices on the list are initialised so
+	don't both checking the init flag.
+
 2004-11-22  Bart Veer  <bartv@ecoscentric.com>
 
 	* src/legacy_dev.c: remove .2ram attributes. These functions do
Index: io/flash/current/src/flash.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/flash/current/src/flash.c,v
retrieving revision 1.26.2.14
diff -u -r1.26.2.14 flash.c
--- io/flash/current/src/flash.c	22 Nov 2004 13:25:23 -0000	1.26.2.14
+++ io/flash/current/src/flash.c	28 Nov 2004 19:02:15 -0000
@@ -154,21 +154,24 @@
   bool moved;
   struct cyg_flash_dev *dev, **previous_next;
 
-  // Place all devices on the list, unsorted for now.
+  // Place all devices that initialised on the list, unsorted for now.
   for (dev = &cyg_flashdevtab[0]; dev != &cyg_flashdevtab_end; dev++) {
-    dev->next  = flash_head;
-    flash_head = dev;
+    if (dev->init) {
+      dev->next  = flash_head;
+      flash_head = dev;
+    }
   }
   
-  // If there are no devices, abort. This should not happen because of
-  // the constraints on CYGHWR_IO_FLASH_DEVICE.
+  // If there are no devices, abort. This could happen because none
+  // of the devices initialised. 
   if (flash_head == NULL) {
     return false;
   }
 
   // Sort the linked list into ascending order of flash address. Use a
   // primitive ripple sort, but since we don't expect to have many
-  // devices this should be OK.
+  // devices this should be OK. This loop may run safely with just one
+  // entry on the list.
   do {
     moved=false;
     for (dev=flash_head, previous_next=&flash_head; 
@@ -204,10 +207,6 @@
   }
   for (dev = flash_head; dev; dev = dev->next) {
     if ((dev->start <= addr) && (addr <= dev->end)) {
-      if (! dev->init) {
-        *stat = CYG_FLASH_ERR_NOT_INIT;
-        return NULL;
-      }
       return dev;
     }
   }
@@ -299,15 +298,16 @@
   if (!init) return CYG_FLASH_ERR_NOT_INIT;
 
 #if (1 == CYGHWR_IO_FLASH_DEVICE)
-  if (0 == Nth) {
+  if ((0 == Nth) && cyg_flashdevtab[0].init) {
       dev = &(cyg_flashdevtab[0]);
   } else {
       return CYG_FLASH_ERR_INVALID;
   }
 #else
+  // Only initialized devices are on the list.
   for (dev = flash_head; dev && Nth; dev=dev->next, Nth--)
     ;
-  if (!dev || !dev->init) {
+  if (!dev) {
       return CYG_FLASH_ERR_INVALID;
   }
 #endif

-- 
Bart Veer                       eCos Configuration Architect
http://www.ecoscentric.com/     The eCos and RedBoot experts


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]