On Wed, 2009-09-02 at 12:25 -0400, Tom Pothier wrote:
> The webrev is now also available on opensolaris.org:
>
> http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
>
> thx,
> -t
Tom,
And now been through the remainder of the code. Comments below.
Thanks,
-scott
==usr/src/uts/common/os/fm.c==
1269 fm_fmri_hc_create(nvlist_t *fmri, int version, const nvlist_t *auth,
...
1322 pairs[i] = fm_nvlist_create(nva);
1323 if (nvlist_add_string(pairs[i], FM_FMRI_HC_NAME, hcname)
!= 0 ||
1324 nvlist_add_string(pairs[i], FM_FMRI_HC_ID, hcid) != 0)
{
1325 atomic_add_64(
1326 &erpt_kstat_data.fmri_set_failed.value.ui64,
1);
1327 return;
1328 }
...
1359 if (nvlist_add_nvlist_array(fmri, FM_FMRI_HC_LIST, pairs,
1360 npairs + n) != 0) {
1361 atomic_add_64(&erpt_kstat_data.fmri_set_failed.value.ui64,
1);
1362 return;
1363 }
1364
1365 for (i = 0; i < npairs + n; i++) {
1366 if (pairs[i] != NULL)
1367 fm_nvlist_destroy(pairs[i], FM_NVA_RETAIN);
1368 }
Possible memory leak here? You're walking the list of bboards,
creating an nvlist pair for each. Suppose you're half way into the
walk and the fm_nvlist_create() at 1322 fails. This'll cause failures
for the nvlist_add_string() calls at 1323 & 1324. Then a stat is
bumped and return. Any successful fm_nvlist_create()s for pairs[i]
are left dangling.
I think the check for NULL at 1366 is unnecessary. If you've gotten
that far, all of the pairs[i] pointers are valid.
You may want to look at fm_fmri_hc_set(), which is doing something
similar to this function.
==usr/src/uts/i86pc/cpu/amd_opteron/ao_mca.c==
Nit. The 'fma_compat' variable seems unnecessary as it's
set as !x86gentopo_legacy, an externally available variable.
FWIW, using the extern is the route taken in authamd_main.c,
gcpu_mca.c, and gintel_main.c.
==usr/src/uts/i86pc/cpu/generic_cpu/gcpu_mca.c==
505 gcpu_fmri_create(cmi_hdl_t hdl, nv_alloc_t *nva)
...
512 if (!x86gentopo_legacy) {
513 fmri = cmi_hdl_smb_bboard(hdl);
514
515 if (fmri == NULL) {
516 if (panicstr) {
517 fm_nvlist_destroy(nvl, FM_NVA_RETAIN);
518 nv_alloc_reset(nva);
519 } else {
520 fm_nvlist_destroy(nvl, FM_NVA_FREE);
521 }
522 return (NULL);
523 }
524
525 fm_fmri_hc_create(nvl, FM_HC_SCHEME_VERSION,
526 NULL, NULL, fmri, 3,
527 "chip", cmi_hdl_smb_chipid(hdl),
528 "core", cmi_hdl_coreid(hdl),
529 "strand", cmi_hdl_strandid(hdl));
530 } else {
531 fm_fmri_hc_set(nvl, FM_HC_SCHEME_VERSION, NULL, NULL, 4,
532 "motherboard", 0,
533 "chip", cmi_hdl_chipid(hdl),
534 "core", cmi_hdl_coreid(hdl),
535 "strand", cmi_hdl_strandid(hdl));
536 }
Curious...why do the checks at 515 and 516 only apply for
generic enumeration? Why not legacy mode too?
==usr/src/uts/i86pc/cpu/genuineintel/gintel_main.c==
297 gintel_gentopo_ereport_create_resource_elem(cmi_hdl_t hdl, nv_alloc_t *nva,
298 mc_unum_t *unump)
299 {
300 nvlist_t *nvl, *snvl;
301 nvlist_t *board_list = NULL;
302 board_list = cmi_hdl_smb_bboard(hdl);
303 /*
304 * do we need to duplicate the nvlist here?
305 */
306 if (board_list == NULL) {
307 return (NULL);
308 }
Has the question at 304 been answered?
==usr/src/uts/i86pc/os/cmi_hw.c==
1287 hdl->cmih_smb_bboard = fm_smb_bboard(strand_apicid);
1288 if (hdl->cmih_smb_bboard == NULL)
1289 cmn_err(CE_WARN, "Read smbios boards failed");
Please add a comment that if 1288 is true, topo reverts to
legacy mode (via the calls to fm_smb_bboard() and smb_bboard()
in fmsmb.c)
==usr/src/uts/intel/os/fmsmb.c==
In one of my prior emails, I had comments on this file.
Noticed a few more things the 2nd time around.
52 typedef enum baseb {
53 BB_BAD = 0, /* There is no bb value 0 */
54 BB_UNKOWN, /* Unknown */
55 BB_OTHER, /* Other */
...
Comment referencing where enum values come from (DMTF) would
be nice. "BB_UNKOWN" --> "BB_UNKNOWN".
69 static struct bboard_type {
70 bbd_t baseb;
71 const char *name;
72 } bbd_type[] = {
73 {BB_BAD, NULL},
74 {BB_UNKOWN, "unknown"},
75 {BB_OTHER, "other"},
76 {BB_BLADE, "blade"},
77 {BB_CONNSW, "connswitch"},
78 {BB_SMM, "smmodule"},
79 {BB_PROCMOD, "cpuboard"},
80 {BB_IOMOD, "ioboard"},
81 {BB_MEMMOD, "memboard"},
82 {BB_DBOARD, "systemboard"},
83 {BB_MBOARD, "motherboard"},
84 {BB_PROCMMOD, "systemboard"},
85 {BB_PROCIOMOD, "systemboard"},
86 {BB_ICONNBD, "iconnboard"}
87 };
...
So this maps a baseboard type to an FMA hc scheme canonical
name. Note that several of the strings are no in the hc
names list (other, smmodule, iconnboard, etc). While they're
not used in the code today, it may be better not to map
these to bogus names. "BB_UNKOWN" --> "BB_UNKNOWN".
Also, in x86pi_bboard, SMB_BBT_BLADE is equated to "systemboard",
but here "blade". Disconnect?
108 static smbs_cnt_t *
109 smb_create_strcnt(int count)
110 {
111 smbs_cnt_t *types = NULL;
112 int i, j;
113
114 types = kmem_zalloc(sizeof (smbs_cnt_t), KM_SLEEP);
115 if (types == NULL) {
116 cmn_err(CE_WARN, "Can't allocate smb strcnt memory");
117 return (NULL);
118 }
You're using the KM_SLEEP flag here, so the memory alloc
is guaranteed to succeed (see kmem_alloc(9F)). The
checks from 115-118 are unnecessary. Ripple this through the
rest of the smb_create_strcnt() function.
931 if (find_matching_proc(shp, strand_apicid,
932 bb_smbid, proc_hdl, is_proc)) {
933 fmri = fm_nvlist_create(NULL);
934 if (fmri == NULL) {
935 cmn_err(CE_WARN, "Can't allocate fmri
mem");
936 goto bad;
937 }
938 /*
939 * find parent by walking the cont_by_id
940 */
941 rc = smb_get_bb_fmri(shp, fmri, bb_smbid,
bbstypes);
942 smb_free_strcnt(bbstypes, bb_strcnt);
943 if (rc == 0) {
944 return (fmri);
945 } else
946 goto bad;
947 }
948
949 }
950
951 cmn_err(CE_NOTE, "Can't get bboards");
952 smb_free_strcnt(bbstypes, bb_strcnt);
953 bad:
If the check at 934 is true, do you need to free bbstypes
before line 936?
_______________________________________________
fm-discuss mailing list
fm-discuss@opensolaris.org
