Re: free() sizes for ahc(4)

2019-05-14 Thread Ted Unangst 
Miod Vallat wrote:
> Note ahc_set_name() gets invoked with the dv_xname field of a struct
> device, so it's not a good idea to free anything, should it be invoked
> more than once.

nice catch.

Re: free() sizes for ahc(4)

2019-05-14 Thread Jan Klemkow 
On Tue, May 14, 2019 at 10:35:22AM +, Miod Vallat wrote:
> Note ahc_set_name() gets invoked with the dv_xname field of a struct
> device, so it's not a good idea to free anything, should it be invoked
> more than once.
> 
> Tested on:
> ahc0 at pci0 dev 1 function 0 "Adaptec AIC-7880" rev 0x00: irq 8
> ahc0: Host Adapter Bios disabled.  Using default SCSI device parameters
> scsibus0 at ahc0: 16 targets, initiator 7
> sd0 at scsibus0 targ 1 lun 0:  SCSI3 0/direct 
> fixed serial.SGI_IBM_DNES-318350Y_AK0T7943
> sd0: 17364MB, 512 bytes/sector, 35563040 sectors
> cd0 at scsibus0 targ 4 lun 0:  SCSI2 5/cdrom 
> removable
> ahc1 at pci0 dev 2 function 0 "Adaptec AIC-7880" rev 0x00: irq 9
> ahc1: Host Adapter Bios disabled.  Using default SCSI device parameters
> scsibus1 at ahc1: 16 targets, initiator 7

OK Jan
 
> Index: dev/ic/aic7xxx.c
> ===
> RCS file: /OpenBSD/src/sys/dev/ic/aic7xxx.c,v
> retrieving revision 1.93
> diff -u -p -u -p -r1.93 aic7xxx.c
> --- dev/ic/aic7xxx.c  12 Dec 2017 12:33:36 -  1.93
> +++ dev/ic/aic7xxx.c  14 May 2019 10:28:10 -
> @@ -1688,7 +1688,7 @@ ahc_free_tstate(struct ahc_softc *ahc, u
>   scsi_id += 8;
>   tstate = ahc->enabled_targets[scsi_id];
>   if (tstate != NULL)
> - free(tstate, M_DEVBUF, 0);
> + free(tstate, M_DEVBUF, sizeof(*tstate));
>   ahc->enabled_targets[scsi_id] = NULL;
>  }
>  #endif
> @@ -3957,8 +3957,6 @@ ahc_set_unit(struct ahc_softc *ahc, int 
>  void
>  ahc_set_name(struct ahc_softc *ahc, char *name)
>  {
> - if (ahc->name != NULL)
> - free(ahc->name, M_DEVBUF, 0);
>   ahc->name = name;
>  }
>  
> @@ -3997,21 +3995,21 @@ ahc_free(struct ahc_softc *ahc)
>   lstate = tstate->enabled_luns[j];
>   if (lstate != NULL) {
> /*xpt_free_path(lstate->path);*/
> - free(lstate, M_DEVBUF, 0);
> + free(lstate, M_DEVBUF, sizeof(*lstate));
>   }
>   }
>  #endif
> - free(tstate, M_DEVBUF, 0);
> + free(tstate, M_DEVBUF, sizeof(*tstate));
>   }
>   }
>  #ifdef AHC_TARGET_MODE
>   if (ahc->black_hole != NULL) {
> /*xpt_free_path(ahc->black_hole->path);*/
> - free(ahc->black_hole, M_DEVBUF, 0);
> + free(ahc->black_hole, M_DEVBUF, sizeof(*ahc->black_hole));
>   }
>  #endif
>   if (ahc->seep_config != NULL)
> - free(ahc->seep_config, M_DEVBUF, 0);
> + free(ahc->seep_config, M_DEVBUF, sizeof(*ahc->seep_config));
>   return;
>  }
>  
> @@ -4329,7 +4327,7 @@ ahc_fini_scbdata(struct ahc_softc *ahc)
>   ahc_freedmamem(ahc->parent_dmat, PAGE_SIZE,
>   sg_map->sg_dmamap, (caddr_t)sg_map->sg_vaddr,
>   _map->sg_dmasegs, sg_map->sg_nseg);
> - free(sg_map, M_DEVBUF, 0);
> + free(sg_map, M_DEVBUF, sizeof(*sg_map));
>   }
>   }
>   /*FALLTHROUGH*/
> @@ -4350,8 +4348,10 @@ ahc_fini_scbdata(struct ahc_softc *ahc)
>   case 0:
>   break;
>   }
> - if (scb_data->scbarray != NULL)
> - free(scb_data->scbarray, M_DEVBUF, 0);
> + if (scb_data->scbarray != NULL) {
> + free(scb_data->scbarray, M_DEVBUF,
> + AHC_SCB_MAX_ALLOC * sizeof(struct scb));
> + }
>  }
>  
>  void
> @@ -4383,7 +4383,7 @@ ahc_alloc_scbs(struct ahc_softc *ahc)
>(caddr_t *)_map->sg_vaddr, _map->sg_physaddr,
>_map->sg_dmasegs, _map->sg_nseg, 
> ahc_name(ahc),
>"SG space") < 0) {
> - free(sg_map, M_DEVBUF, 0);
> + free(sg_map, M_DEVBUF, sizeof(*sg_map));
>   return;
>   }
>  
> @@ -6859,7 +6859,7 @@ ahc_handle_en_lun(struct ahc_softc *ahc,
>xpt_path_target_id(ccb->ccb_h.path),
>xpt_path_lun_id(ccb->ccb_h.path));
>   if (status != CAM_REQ_CMP) {
> - free(lstate, M_DEVBUF, 0);
> + free(lstate, M_DEVBUF, sizeof(*lstate));
>   xpt_print_path(ccb->ccb_h.path);
>   printf("Couldn't allocate path\n");
>   ccb->ccb_h.status = CAM_RESRC_UNAVAIL;
> @@ -6980,7 +6980,7 @@ ahc_handle_en_lun(struct ahc_softc *ahc,
>   xpt_print_path(ccb->ccb_h.path);
>   printf("Target mode disabled\n");
>   xpt_free_path(lstate->path);
> - free(lstate, M_DEVBUF, 0);
> + free(lstate, M_DEVBUF, sizeof(*lstate));
>  
>   ahc_pause(ahc);
>   /* Can we clean up the target too? */
> Index:

free() sizes for ahc(4)

2019-05-14 Thread Miod Vallat 
Note ahc_set_name() gets invoked with the dv_xname field of a struct
device, so it's not a good idea to free anything, should it be invoked
more than once.

Tested on:
ahc0 at pci0 dev 1 function 0 "Adaptec AIC-7880" rev 0x00: irq 8
ahc0: Host Adapter Bios disabled.  Using default SCSI device parameters
scsibus0 at ahc0: 16 targets, initiator 7
sd0 at scsibus0 targ 1 lun 0:  SCSI3 0/direct 
fixed serial.SGI_IBM_DNES-318350Y_AK0T7943
sd0: 17364MB, 512 bytes/sector, 35563040 sectors
cd0 at scsibus0 targ 4 lun 0:  SCSI2 5/cdrom 
removable
ahc1 at pci0 dev 2 function 0 "Adaptec AIC-7880" rev 0x00: irq 9
ahc1: Host Adapter Bios disabled.  Using default SCSI device parameters
scsibus1 at ahc1: 16 targets, initiator 7

Index: dev/ic/aic7xxx.c
===
RCS file: /OpenBSD/src/sys/dev/ic/aic7xxx.c,v
retrieving revision 1.93
diff -u -p -u -p -r1.93 aic7xxx.c
--- dev/ic/aic7xxx.c12 Dec 2017 12:33:36 -  1.93
+++ dev/ic/aic7xxx.c14 May 2019 10:28:10 -
@@ -1688,7 +1688,7 @@ ahc_free_tstate(struct ahc_softc *ahc, u
scsi_id += 8;
tstate = ahc->enabled_targets[scsi_id];
if (tstate != NULL)
-   free(tstate, M_DEVBUF, 0);
+   free(tstate, M_DEVBUF, sizeof(*tstate));
ahc->enabled_targets[scsi_id] = NULL;
 }
 #endif
@@ -3957,8 +3957,6 @@ ahc_set_unit(struct ahc_softc *ahc, int 
 void
 ahc_set_name(struct ahc_softc *ahc, char *name)
 {
-   if (ahc->name != NULL)
-   free(ahc->name, M_DEVBUF, 0);
ahc->name = name;
 }
 
@@ -3997,21 +3995,21 @@ ahc_free(struct ahc_softc *ahc)
lstate = tstate->enabled_luns[j];
if (lstate != NULL) {
  /*xpt_free_path(lstate->path);*/
-   free(lstate, M_DEVBUF, 0);
+   free(lstate, M_DEVBUF, sizeof(*lstate));
}
}
 #endif
-   free(tstate, M_DEVBUF, 0);
+   free(tstate, M_DEVBUF, sizeof(*tstate));
}
}
 #ifdef AHC_TARGET_MODE
if (ahc->black_hole != NULL) {
  /*xpt_free_path(ahc->black_hole->path);*/
-   free(ahc->black_hole, M_DEVBUF, 0);
+   free(ahc->black_hole, M_DEVBUF, sizeof(*ahc->black_hole));
}
 #endif
if (ahc->seep_config != NULL)
-   free(ahc->seep_config, M_DEVBUF, 0);
+   free(ahc->seep_config, M_DEVBUF, sizeof(*ahc->seep_config));
return;
 }
 
@@ -4329,7 +4327,7 @@ ahc_fini_scbdata(struct ahc_softc *ahc)
ahc_freedmamem(ahc->parent_dmat, PAGE_SIZE,
sg_map->sg_dmamap, (caddr_t)sg_map->sg_vaddr,
_map->sg_dmasegs, sg_map->sg_nseg);
-   free(sg_map, M_DEVBUF, 0);
+   free(sg_map, M_DEVBUF, sizeof(*sg_map));
}
}
/*FALLTHROUGH*/
@@ -4350,8 +4348,10 @@ ahc_fini_scbdata(struct ahc_softc *ahc)
case 0:
break;
}
-   if (scb_data->scbarray != NULL)
-   free(scb_data->scbarray, M_DEVBUF, 0);
+   if (scb_data->scbarray != NULL) {
+   free(scb_data->scbarray, M_DEVBUF,
+   AHC_SCB_MAX_ALLOC * sizeof(struct scb));
+   }
 }
 
 void
@@ -4383,7 +4383,7 @@ ahc_alloc_scbs(struct ahc_softc *ahc)
 (caddr_t *)_map->sg_vaddr, _map->sg_physaddr,
 _map->sg_dmasegs, _map->sg_nseg, 
ahc_name(ahc),
 "SG space") < 0) {
-   free(sg_map, M_DEVBUF, 0);
+   free(sg_map, M_DEVBUF, sizeof(*sg_map));
return;
}
 
@@ -6859,7 +6859,7 @@ ahc_handle_en_lun(struct ahc_softc *ahc,
 xpt_path_target_id(ccb->ccb_h.path),
 xpt_path_lun_id(ccb->ccb_h.path));
if (status != CAM_REQ_CMP) {
-   free(lstate, M_DEVBUF, 0);
+   free(lstate, M_DEVBUF, sizeof(*lstate));
xpt_print_path(ccb->ccb_h.path);
printf("Couldn't allocate path\n");
ccb->ccb_h.status = CAM_RESRC_UNAVAIL;
@@ -6980,7 +6980,7 @@ ahc_handle_en_lun(struct ahc_softc *ahc,
xpt_print_path(ccb->ccb_h.path);
printf("Target mode disabled\n");
xpt_free_path(lstate->path);
-   free(lstate, M_DEVBUF, 0);
+   free(lstate, M_DEVBUF, sizeof(*lstate));
 
ahc_pause(ahc);
/* Can we clean up the target too? */
Index: dev/ic/aic7xxx_seeprom.c
===
RCS file: /OpenBSD/src/sys/dev/ic/aic7xxx_seeprom.c,v
retrieving revision 1.7
diff