Change definition of NO_PIN to 1+PIN_MAX (#1231)

Fixes bug #1228 that gpio 0 could not be used by the linuxgpio system.

* Add sanity checks
* Loop over defined pins only

Co-authored-by: Stefan Rueger <stefan.rueger@urclocks.com>

src/avr.c

@@ -769,7 +769,7 @@ int avr_write_byte_default(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM
        */
       pgm->pgm_led(pgm, OFF);
       pmsg_info("this device must be powered off and back on to continue\n");
-      if (pgm->pinno[PPI_AVR_VCC]) {
+      if ((pgm->pinno[PPI_AVR_VCC] & PIN_MASK) <= PIN_MAX) {
         pmsg_info("attempting to do this now ...\n");
         pgm->powerdown(pgm);
         usleep(250000);

src/avrftdi.c

@@ -224,6 +224,9 @@ static int set_frequency(avrftdi_t* ftdi, uint32_t freq)
  * here.
  */
 static int set_pin(const PROGRAMMER *pgm, int pinfunc, int value) {
+	if(pinfunc < 0 || pinfunc >= N_PINS)
+		return -1;
+
 	avrftdi_t* pdata = to_pdata(pgm);
 	struct pindef_t pin = pgm->pin[pinfunc];
 	

src/bitbang.c

@@ -608,8 +608,13 @@ int bitbang_initialize(const PROGRAMMER *pgm, const AVRPART *p) {
   return 0;
 }
 
-static int verify_pin_assigned(const PROGRAMMER *pgm, int pin, char *desc) {
+static int verify_pin_assigned(const PROGRAMMER *pgm, int pinfunc, char *desc) {
-  if (pgm->pinno[pin] == 0) {
+  if(pinfunc < 0 || pinfunc >= N_PINS) {
+    pmsg_error("invalid pin function number %d\n", pinfunc);
+    return -1;
+  }
+
+  if ((pgm->pinno[pinfunc] & PIN_MASK) > PIN_MAX) {
     pmsg_error("no pin has been assigned for %s\n", desc);
     return -1;
   }

src/buspirate.c

@@ -1198,8 +1198,12 @@ static void buspirate_bb_enable(PROGRAMMER *pgm, const AVRPART *p) {
 */
 static int buspirate_bb_getpin(const PROGRAMMER *pgm, int pinfunc) {
 	unsigned char buf[10];
-	int value = 0;
+	int pin, value = 0;
-	int pin = pgm->pinno[pinfunc];
+
+	if(pinfunc < 0 || pinfunc >= N_PINS)
+		return -1;
+
+	pin = pgm->pinno[pinfunc];
 
 	if (pin & PIN_INVERSE) {
 		pin &= PIN_MASK;
@@ -1261,6 +1265,9 @@ static int buspirate_bb_setpin_internal(const PROGRAMMER *pgm, int pin, int valu
 }
 
 static int buspirate_bb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) {
+	if(pinfunc < 0 || pinfunc >= N_PINS)
+		return -1;
+
 	return buspirate_bb_setpin_internal(pgm, pgm->pinno[pinfunc], value);
 }
 

src/config_gram.y

@@ -44,7 +44,7 @@ int yylex(void);
 int yyerror(char * errmsg, ...);
 int yywarning(char * errmsg, ...);
 
-static int assign_pin(int pinno, TOKEN * v, int invert);
+static int assign_pin(int pinfunc, TOKEN *v, int invert);
 static int assign_pin_list(int invert);
 static int which_opcode(TOKEN * opcode);
 static int parse_cmdbits(OPCODE * op, int opnum);
@@ -1550,11 +1550,13 @@ static char * vtypestr(int type)
 #endif
 
 
-static int assign_pin(int pinno, TOKEN * v, int invert)
+static int assign_pin(int pinfunc, TOKEN *v, int invert) {
-{
+  if(pinfunc < 0 || pinfunc >= N_PINS) {
-  int value;
+    yyerror("pin function must be in the range [0, %d]", N_PINS-1);
+    return -1;
+  }
 
-  value = v->value.number;
+  int value = v->value.number;
   free_token(v);
 
   if ((value < PIN_MIN) || (value > PIN_MAX)) {
@@ -1562,7 +1564,7 @@ static int assign_pin(int pinno, TOKEN * v, int invert)
     return -1;
   }
 
-  pin_set_value(&(current_prog->pin[pinno]), value, invert);
+  pin_set_value(&(current_prog->pin[pinfunc]), value, invert);
 
   return 0;
 }
@@ -1573,7 +1575,12 @@ static int assign_pin_list(int invert)
   int pin;
   int rv = 0;
 
-  current_prog->pinno[pin_name] = 0;
+  if(pin_name < 0 || pin_name >= N_PINS) {
+    yyerror("pin_name should be in the range [0, %d]", N_PINS-1);
+    return -1;
+  }
+
+  current_prog->pinno[pin_name] = NO_PIN;
   while (lsize(number_list)) {
     t = lrmv_n(number_list, 1);
     if (rv == 0) {

src/libavrdude.h

@@ -434,7 +434,7 @@ enum {
 /** Number of pins in each element of the bitfield */
 #define PIN_FIELD_ELEMENT_SIZE (sizeof(pinmask_t) * 8)
 /** Numer of elements to store the complete bitfield of all pins */
-#define PIN_FIELD_SIZE ((PIN_MAX + PIN_FIELD_ELEMENT_SIZE)/PIN_FIELD_ELEMENT_SIZE)
+#define PIN_FIELD_SIZE ((PIN_MAX+1 + PIN_FIELD_ELEMENT_SIZE-1)/PIN_FIELD_ELEMENT_SIZE)
 
 /**
  * This sets the corresponding bits to 1 or 0, the inverse mask is used to invert the value in necessary.
@@ -832,6 +832,8 @@ typedef struct programmer_t {
   char flag;                    // For use by pgm->initpgm()
 } PROGRAMMER;
 
+#define NO_PIN   (PIN_MAX + 1U) // Magic pinno[] value for unused pins
+
 #ifdef __cplusplus
 extern "C" {
 #endif

src/linuxgpio.c

@@ -49,8 +49,6 @@
  * GPIO user space helpers
  * The following functions are acting on an "unsigned gpio" argument, which corresponds to the
  * gpio numbering scheme in the kernel (starting from 0).
- * The higher level functions use "int pin" to specify the pins with an offset of 1:
- * gpio = pin - 1;
  */
 
 #define GPIO_DIR_IN	0
@@ -151,24 +149,19 @@ static int linuxgpio_fds[N_GPIO] ;
 
 
 static int linuxgpio_setpin(const PROGRAMMER *pgm, int pinfunc, int value) {
-  int r;
+  if(pinfunc < 0 || pinfunc >= N_PINS)
-  int pin = pgm->pinno[pinfunc]; // TODO
-
-  if (pin & PIN_INVERSE)
-  {
-    value  = !value;
-    pin   &= PIN_MASK;
-  }
-
-  if ( linuxgpio_fds[pin] < 0 )
     return -1;
 
-  if (value)
+  unsigned pin = pgm->pinno[pinfunc];
-    r = write(linuxgpio_fds[pin], "1", 1);
+  if (pin & PIN_INVERSE)
-  else
+    value = !value;
-    r = write(linuxgpio_fds[pin], "0", 1);
+  pin &= PIN_MASK;
 
-  if (r!=1) return -1;
+  if (pin > PIN_MAX || linuxgpio_fds[pin] < 0)
+    return -1;
+
+  if (write(linuxgpio_fds[pin], value? "1": "0", 1) != 1)
+    return -1;
 
   if (pgm->ispdelay > 1)
     bitbang_delay(pgm->ispdelay);
@@ -177,37 +170,33 @@ static int linuxgpio_setpin(const PROGRAMMER *pgm, int pinfunc, int value) {
 }
 
 static int linuxgpio_getpin(const PROGRAMMER *pgm, int pinfunc) {
-  unsigned char invert=0;
+  if(pinfunc < 0 || pinfunc >= N_PINS)
-  char c;
+    return -1;
-  int pin = pgm->pinno[pinfunc]; // TODO
 
-  if (pin & PIN_INVERSE)
+  unsigned int pin = pgm->pinno[pinfunc];
-  {
+  int invert = !!(pin & PIN_INVERSE);
-    invert = 1;
   pin &= PIN_MASK;
-  }
 
-  if ( linuxgpio_fds[pin] < 0 )
+  if(pin > PIN_MAX || linuxgpio_fds[pin] < 0)
     return -1;
 
   if(lseek(linuxgpio_fds[pin], 0, SEEK_SET) < 0)
     return -1;
 
+  char c;
   if(read(linuxgpio_fds[pin], &c, 1) != 1)
     return -1;
 
-  if (c=='0')
+  return c=='0'? 0+invert: c=='1'? 1-invert: -1;
-    return 0+invert;
-  else if (c=='1')
-    return 1-invert;
-  else
-    return -1;
 }
 
 static int linuxgpio_highpulsepin(const PROGRAMMER *pgm, int pinfunc) {
-  int pin = pgm->pinno[pinfunc]; // TODO
+  if(pinfunc < 0 || pinfunc >= N_PINS)
+    return -1;
 
-  if ( linuxgpio_fds[pin & PIN_MASK] < 0 )
+  unsigned int pin = pgm->pinno[pinfunc] & PIN_MASK;
+
+  if (pin > PIN_MAX || linuxgpio_fds[pin] < 0 )
     return -1;
 
   linuxgpio_setpin(pgm, pinfunc, 1);
@@ -250,20 +239,9 @@ static int linuxgpio_open(PROGRAMMER *pgm, const char *port) {
 
   for (i=0; i<N_GPIO; i++)
     linuxgpio_fds[i] = -1;
-  //Avrdude assumes that if a pin number is 0 it means not used/available
+  // Avrdude assumes that if a pin number is invalid it means not used/available
-  //this causes a problem because 0 is a valid GPIO number in Linux sysfs.
+  for (i = 1; i < N_PINS; i++) { // The pin enumeration in libavrdude.h starts with PPI_AVR_VCC = 1
-  //To avoid annoying off by one pin numbering we assume SCK, SDO, SDI 
+    if ((pgm->pinno[i] & PIN_MASK) <= PIN_MAX) {
-  //and RESET pins are always defined in avrdude.conf, even as 0. If they're
-  //not programming will not work anyway. The drawbacks of this approach are
-  //that unwanted toggling of GPIO0 can occur and that other optional pins
-  //mostry LED status, can't be set to GPIO0. It can be fixed when a better 
-  //solution exists.
-  for (i=0; i<N_PINS; i++) {
-    if ( (pgm->pinno[i] & PIN_MASK) != 0 ||
-         i == PIN_AVR_RESET ||
-         i == PIN_AVR_SCK   ||
-         i == PIN_AVR_SDO   ||
-         i == PIN_AVR_SDI ) {
         pin = pgm->pinno[i] & PIN_MASK;
         if ((r=linuxgpio_export(pin)) < 0) {
             pmsg_ext_error("cannot export GPIO %d, already exported/busy?: %s",
@@ -333,13 +311,15 @@ static void linuxgpio_close(PROGRAMMER *pgm)
   for (i=0; i<N_GPIO; i++) {
     if (linuxgpio_fds[i] >= 0 && i != reset_pin) {
        close(linuxgpio_fds[i]);
+       linuxgpio_fds[i] = -1;
        linuxgpio_dir_in(i);
        linuxgpio_unexport(i);
     }
   }
   //configure RESET as input, if there's external pull up it will go high
-  if (linuxgpio_fds[reset_pin] >= 0) {
+  if(reset_pin <= PIN_MAX && linuxgpio_fds[reset_pin] >= 0) {
     close(linuxgpio_fds[reset_pin]);
+    linuxgpio_fds[reset_pin] = -1;
     linuxgpio_dir_in(reset_pin);
     linuxgpio_unexport(reset_pin);
   }

src/linuxspi.c

@@ -134,7 +134,7 @@ static int linuxspi_reset_mcu(const PROGRAMMER *pgm, bool active) {
 #endif
     if (ret == -1) {
         ret = -errno;
-        pmsg_ext_error("unable to set GPIO line %d value: %s\n", pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE, strerror(errno));
+        pmsg_ext_error("unable to set GPIO line %d value: %s\n", pgm->pinno[PIN_AVR_RESET] & PIN_MASK, strerror(errno));
         return ret;
     }
 
@@ -200,7 +200,7 @@ static int linuxspi_open(PROGRAMMER *pgm, const char *pt) {
 
     strcpy(req.consumer_label, progname);
     req.lines = 1;
-    req.lineoffsets[0] = pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE;
+    req.lineoffsets[0] = pgm->pinno[PIN_AVR_RESET] & PIN_MASK;
     req.default_values[0] = !!(pgm->pinno[PIN_AVR_RESET] & PIN_INVERSE);
     req.flags = GPIOHANDLE_REQUEST_OUTPUT;
 
@@ -212,7 +212,7 @@ static int linuxspi_open(PROGRAMMER *pgm, const char *pt) {
         struct gpio_v2_line_request reqv2;
 
         memset(&reqv2, 0, sizeof(reqv2));
-        reqv2.offsets[0] = pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE;
+        reqv2.offsets[0] = pgm->pinno[PIN_AVR_RESET] & PIN_MASK;
         strncpy(reqv2.consumer, progname, sizeof(reqv2.consumer) - 1);
         reqv2.config.flags = GPIO_V2_LINE_FLAG_OUTPUT;
         reqv2.config.num_attrs = 1;
@@ -228,7 +228,7 @@ static int linuxspi_open(PROGRAMMER *pgm, const char *pt) {
 #endif
     if (ret == -1) {
         ret = -errno;
-        pmsg_ext_error("unable to get GPIO line %d. %s\n", pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE, strerror(errno));
+        pmsg_ext_error("unable to get GPIO line %d. %s\n", pgm->pinno[PIN_AVR_RESET] & PIN_MASK, strerror(errno));
         goto close_gpiochip;
     }
 

src/par.c

@@ -102,12 +102,19 @@ static int par_setpin_internal(const PROGRAMMER *pgm, int pin, int value) {
 }
 
 static int par_setpin(const PROGRAMMER * pgm, int pinfunc, int value) {
+  if(pinfunc < 0 || pinfunc >= N_PINS)
+    return -1;
+
   return par_setpin_internal(pgm, pgm->pinno[pinfunc], value);
 }
 
 static void par_setmany(const PROGRAMMER *pgm, int pinfunc, int value) {
-  int pin, mask;
+  int pin, mask, pinset;
-  int pinset = pgm->pinno[pinfunc];
+
+  if(pinfunc < 0 || pinfunc >= N_PINS)
+    return;
+
+  pinset = pgm->pinno[pinfunc];
 
   /* mask is anything non-pin - needs to be applied to each par_setpin to preserve inversion */
   mask = pinset & (~PIN_MASK);
@@ -119,9 +126,12 @@ static void par_setmany(const PROGRAMMER *pgm, int pinfunc, int value) {
 }
 
 static int par_getpin(const PROGRAMMER * pgm, int pinfunc) {
-  int value;
+  int value, inverted, pin;
-  int inverted;
+
-  int pin = pgm->pinno[pinfunc];
+  if(pinfunc < 0 || pinfunc >= N_PINS)
+    return -1;
+
+  pin = pgm->pinno[pinfunc];
 
   inverted = pin & PIN_INVERSE;
   pin &= PIN_MASK;
@@ -147,8 +157,12 @@ static int par_getpin(const PROGRAMMER * pgm, int pinfunc) {
 
 
 static int par_highpulsepin(const PROGRAMMER *pgm, int pinfunc) {
-  int inverted;
+  int inverted, pin;
-  int pin = pgm->pinno[pinfunc];
+
+  if(pinfunc < 0 || pinfunc >= N_PINS)
+    return -1;
+
+  pin = pgm->pinno[pinfunc];
 
   inverted = pin & PIN_INVERSE;
   pin &= PIN_MASK;

src/pgm.c

@@ -89,7 +89,7 @@ PROGRAMMER *pgm_new(void) {
 
   // Clear pin array
   for(int i=0; i<N_PINS; i++) {
-    pgm->pinno[i] = 0;
+    pgm->pinno[i] = NO_PIN;
     pin_clear_all(&(pgm->pin[i]));
   }
 

src/pindefs.c

@@ -61,7 +61,7 @@ void pin_clear_all(struct pindef_t * const pindef) {
 static int pin_fill_old_pinno(const struct pindef_t * const pindef, unsigned int * const pinno) {
   bool found = false;
   int i;
-  for(i = 0; i < PIN_MAX; i++) {
+  for(i = 0; i <= PIN_MAX; i++) {
     if(pindef->mask[i / PIN_FIELD_ELEMENT_SIZE] & (1 << (i % PIN_FIELD_ELEMENT_SIZE))) {
       if(found) {
         pmsg_error("multiple pins found\n"); // TODO
@@ -94,7 +94,7 @@ static int pin_fill_old_pinlist(const struct pindef_t * const pindef, unsigned i
       }
       if (pindef->mask[i] == 0) {
         /* this pin function is not using any pins */
-        *pinno = 0;
+        *pinno = NO_PIN;
       } else if(pindef->mask[i] == pindef->inverse[i]) {  /* all set bits in mask are set in inverse */
         *pinno = pindef->mask[i];
         *pinno |= PIN_INVERSE;

src/serbb_posix.c

@@ -73,6 +73,10 @@ static char *serpins[DB9PINS + 1] =
 static int serbb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) {
   unsigned int	ctl;
   int           r;
+
+  if(pinfunc < 0 || pinfunc >= N_PINS)
+    return -1;
+
   int pin = pgm->pinno[pinfunc]; // get its value
 
   if (pin & PIN_INVERSE)
@@ -130,6 +134,10 @@ static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) {
   unsigned int	ctl;
   unsigned char invert;
   int           r;
+
+  if(pinfunc < 0 || pinfunc >= N_PINS)
+    return -1;
+
   int pin = pgm->pinno[pinfunc]; // get its value
 
   if (pin & PIN_INVERSE)
@@ -177,6 +185,9 @@ static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) {
 }
 
 static int serbb_highpulsepin(const PROGRAMMER *pgm, int pinfunc) {
+  if (pinfunc < 0 || pinfunc >= N_PINS)
+    return -1;
+
   int pin = pgm->pinno[pinfunc]; // replace pin name by its value
 
   if ( (pin & PIN_MASK) < 1 || (pin & PIN_MASK) > DB9PINS )

src/serbb_win32.c

@@ -62,6 +62,9 @@ static int dtr, rts, txd;
 #define DB9PINS 9
 
 static int serbb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) {
+	if(pinfunc < 0 || pinfunc >= N_PINS)
+		return -1;
+
 	int pin = pgm->pinno[pinfunc];
 	HANDLE hComPort = (HANDLE)pgm->fd.pfd;
         LPVOID lpMsgBuf;
@@ -126,6 +129,9 @@ static int serbb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) {
 }
 
 static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) {
+	if(pinfunc < 0 || pinfunc >= N_PINS)
+		return -1;
+
 	int pin = pgm->pinno[pinfunc];
 	HANDLE hComPort = (HANDLE)pgm->fd.pfd;
         LPVOID lpMsgBuf;
@@ -208,6 +214,9 @@ static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) {
 }
 
 static int serbb_highpulsepin(const PROGRAMMER *pgm, int pinfunc) {
+	if(pinfunc < 0 || pinfunc >= N_PINS)
+		return -1;
+
         int pin = pgm->pinno[pinfunc];
         if ( (pin & PIN_MASK) < 1 || (pin & PIN_MASK) > DB9PINS )
           return -1;