linux-kernel - Re: [RFC PATCH 2/4] drivers/bus: Freescale Management Complex (fsl-mc) bus driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201408211330.20869.arnd@arndb.de>
Date:	Thu, 21 Aug 2014 13:30:20 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	German Rivera <German.Rivera@...escale.com>
Cc:	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	stuart.yoder@...escale.com, linuxppc-release@...ux.freescale.net
Subject: Re: [RFC PATCH 2/4] drivers/bus: Freescale Management Complex (fsl-mc) bus driver

On Tuesday 19 August 2014, German Rivera wrote:
> >> + * @dev_node: Node in the container's child list
> >
> > Same here: just use the device model's list management instead if you can,
> > or explain why this is needed.
> >
> We still need to keep a per-bus list of child devices (devices contained 
> in a given DPRC object). Unless I'm missing something,
> I think the device model's list management links together all the 
> devices of the same bus type. We are trying to follow a similar approach 
> to the pci_dev/pci_bus structs.

There are multiple lists in the device handling. device_for_each_child()
should iterate over the children of a particular device using the
klist_children member.

> >> +/**
> >> + * struct fsl_mc_dprc - Data Path Resource Container (DPRC) object
> >> + * @magic: marker to verify identity of this structure
> >> + * @mc_dev: pointer to MC object device object for this DPRC
> >> + * @mutex: mutex to serialize access to the container.
> >> + * @child_device_count: have the count of devices in this DPRC
> >> + * @child_list:     anchor node of list of child devices on this DPRC
> >> + */
> >> +struct fsl_mc_dprc {
> >> +#   define FSL_MC_DPRC_MAGIC   FSL_MC_MAGIC('D', 'P', 'R', 'C')
> >> +    uint32_t magic;
> >> +    struct fsl_mc_device *mc_dev;
> >> +    struct mutex mutex;     /* serializes access to fields below */
> >> +    uint16_t child_device_count;    /* Count of devices in this DPRC */
> >> +    struct list_head child_list;
> >> +};
> >
> > It's not clear what this represents to me. mc_dev presumably already
> > has a list of children, so why not just use a pointer to mc_dev
> > and remove this structure entirely?
> >
> This structure represents the per-bus (per DPRC object) information.
> It is kind of the equivalent to 'struct pci_bus' in the PCI world.
> I have renamed this struct to 'struct fsl_mc_bus'.

Ok, I'll look at the new version when I get back to Germany. I still think
that can remove all members of the current structure and just use the
same structure for fsl_mc_bus and fsl_mc_device. If you really need
a small number of extra members beyond what is in the device, you have
two other choices:

a) put the members into the device structure as well but not use them
   for a device that is not a bus

b) embed the device structure within the bus structure like

	struct fsl_mc_bus {
		int something;
		struct fsl_mc_device;
	};

and then use container_of() to go from the device to the bus where needed
rather than having two objects that are allocated separately. This is
what a lot of other subsystems (not PCI) do. See for instance
platform_device, which often has child devices as well.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/