On 2/16/2023 11:37 PM, Greg Kroah-Hartman wrote:
On Thu, Feb 16, 2023 at 09:40:52AM -0800, Elliot Berman wrote:
On 2/15/2023 10:43 PM, Greg Kroah-Hartman wrote:
On Tue, Feb 14, 2023 at 01:23:25PM -0800, Elliot Berman wrote:
+struct gh_rm {
+ struct device *dev;
What device does this point to?
The platform device.
What platform device? And why a platform device?
This will be used for the dev_printk. It's presently also used for the
reference counting. From your comments below, I'll switch the reference
counting away from this platform device.
+ struct gunyah_resource tx_ghrsc, rx_ghrsc;
+ struct gh_msgq msgq;
+ struct mbox_client msgq_client;
+ struct gh_rm_connection *active_rx_connection;
+ int last_tx_ret;
+
+ struct idr call_idr;
+ struct mutex call_idr_lock;
+
+ struct kmem_cache *cache;
+ struct mutex send_lock;
+ struct blocking_notifier_head nh;
+};
This obviously is the "device" that your system works on, so what are
the lifetime rules of it? Why isn't is just a real 'struct device' in
the system instead of a random memory blob with a pointer to a device?
What controls the lifetime of this structure and where is the reference
counting logic for it?
The lifetime of the structure is bound by the platform device that above
struct device *dev points to. get_gh_rm and put_gh_rm increments the device
ref counter and ensures lifetime of the struct is also extended.
But this really is "your" device, not the platform device. So make it a
real one please as that is how the kernel's driver model works. Don't
hang "magic structures" off of a random struct device and have them
control the lifetime rules of the parent without actually being a device
themself. This should make things simpler overall, not more complex,
and allow you to expose things to userspace properly (right now your
data is totally hidden.)
The "real" device I create here is the miscdev, so I think the
recommendation here is to do refcounting off that miscdev. Is this the
approach you were thinking of?
Thanks,
Elliot
