On 2/15/22 20:07, John Garry wrote: > On 14/02/2022 02:17, Damien Le Moal wrote: >> Introduce the pm8001_ccb_alloc() and pm8001_ccb_free() helpers to >> replace the typical code patterns: >> >> res = pm8001_tag_alloc(pm8001_ha, &ccb_tag); >> if (res) >> ... >> ccb = &pm8001_ha->ccb_info[ccb_tag]; >> ccb->device = pm8001_ha_dev; >> ccb->ccb_tag = ccb_tag; >> ccb->task = task; >> ccb->n_elem = 0;A > > nit: stray 'A' character Weird... How did this pass compile & test ? Will fix that in v4. > >> >> and >> >> ccb->task = NULL; >> ccb->ccb_tag = PM8001_INVALID_TAG; >> pm8001_tag_free(pm8001_ha, tag); > > Isn't it possible to do a "memset struct members", such that we order > the members specifically that we can memset(0) a known region? The > advantage is that adding/removing a member does not require much code to > be touched and possibly missed. Hmmm. I guess we can by moving the buf_prd field at the end of the struct. We would still need the ccb_tag initialization though, so I am not entirely sure if this is worse it... Will try to see how it looks. > >> >> With the simpler function calls: >> >> ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_ha_dev, task); >> if (!ccb) >> ... >> >> and >> >> pm8001_ccb_free(pm8001_ha, ccb); >> >> The pm8001_ccb_alloc() helper ensures that all fields of the ccb info >> structure for the newly allocated tag are all initialized, except the >> buf_prd field. The pm8001_ccb_free() helper clears the initialized >> fields and the ccb tag to ensure that iteration over the adapter >> ccb_info array detects ccbs that are in use. > > >> >> All call site of the pm8001_tag_alloc() function that use a ccb info >> associated with an allocated tag are converted to use the new helpers. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> >> --- > > The comments are not a show stopper, so: > Reviewed-by: John Garry <john.garry@xxxxxxxxxx> > -- Damien Le Moal Western Digital Research
