Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
> I think we are getting our terms confused here. > > "init" usually means module_init(), is that what you are referring to > here? This is just a function that probe calls to, goldfish_pipe_device_v2_init. https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/goldfish_pipe_v2.c#1128 > Why would your probe function know, or care, about versions? Different driver versions allocate different state. > How is your probe function learning about the "version" to use here? writel(PIPE_DRIVER_VERSION, base + PIPE_V2_REG_VERSION); version = readl(base + PIPE_V2_REG_VERSION); https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/goldfish_pipe.c#106 > You have to clean up after your probe function failing before you return > from it. Are you saying that devm_kzalloc does not free memory if probe fails? Is this useful? I can free memory myself then.
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
> I think we are getting our terms confused here. > > "init" usually means module_init(), is that what you are referring to > here? This is just a function that probe calls to, goldfish_pipe_device_v2_init. https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/goldfish_pipe_v2.c#1128 > Why would your probe function know, or care, about versions? Different driver versions allocate different state. > How is your probe function learning about the "version" to use here? writel(PIPE_DRIVER_VERSION, base + PIPE_V2_REG_VERSION); version = readl(base + PIPE_V2_REG_VERSION); https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/goldfish_pipe.c#106 > You have to clean up after your probe function failing before you return > from it. Are you saying that devm_kzalloc does not free memory if probe fails? Is this useful? I can free memory myself then.
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
On Mon, Oct 15, 2018 at 12:06:12PM -0700, Roman Kiryanov wrote: > > > probe does not know what memory to allocate. We have several versions > > > of the driver (with different init) and different versions allocate > > > different state. > > > > I only see one driver here. > > It will be added in "PATCH v3 15/15". There will be two init functions > allocating different states. I think we are getting our terms confused here. "init" usually means module_init(), is that what you are referring to here? probe means when your bus-specific driver's probe function is called because the kernel recognizes your hardware as being present. At module_init time, you should do nothing except register your bus specific driver. At probe time, allocate memory all you want, that is the correct point in time to do it. So yes, if you have two separate drivers, you will have two separate init functions, but both of them should just register the bus specific driver. > > Why does probe not know what to allocate? That is exactly when the > > device is bound to the driver, you have _way_ more information than you > > do at init time. > > We have two versions of the driver. Probe asks for the version and > calls the init function for this version. > I don't want probe to know everything about all versions. Why would your probe function know, or care, about versions? That's up to the driver core. How is your probe function learning about the "version" to use here? > > > >, not init time as what > > > > happens if the hardware is not present yet your driver is loaded? > > > > > > init will have to rollback what it allocated. > > > > But those resources it will sit there wasted until unload happens. And > > unload _never_ happens on a system unless you are a developer working on > > the module. > > If probe fails I expect the kernel to release all resources. Is this > not the case? You have to clean up after your probe function failing before you return from it. thanks, greg k-h
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
On Mon, Oct 15, 2018 at 12:06:12PM -0700, Roman Kiryanov wrote: > > > probe does not know what memory to allocate. We have several versions > > > of the driver (with different init) and different versions allocate > > > different state. > > > > I only see one driver here. > > It will be added in "PATCH v3 15/15". There will be two init functions > allocating different states. I think we are getting our terms confused here. "init" usually means module_init(), is that what you are referring to here? probe means when your bus-specific driver's probe function is called because the kernel recognizes your hardware as being present. At module_init time, you should do nothing except register your bus specific driver. At probe time, allocate memory all you want, that is the correct point in time to do it. So yes, if you have two separate drivers, you will have two separate init functions, but both of them should just register the bus specific driver. > > Why does probe not know what to allocate? That is exactly when the > > device is bound to the driver, you have _way_ more information than you > > do at init time. > > We have two versions of the driver. Probe asks for the version and > calls the init function for this version. > I don't want probe to know everything about all versions. Why would your probe function know, or care, about versions? That's up to the driver core. How is your probe function learning about the "version" to use here? > > > >, not init time as what > > > > happens if the hardware is not present yet your driver is loaded? > > > > > > init will have to rollback what it allocated. > > > > But those resources it will sit there wasted until unload happens. And > > unload _never_ happens on a system unless you are a developer working on > > the module. > > If probe fails I expect the kernel to release all resources. Is this > not the case? You have to clean up after your probe function failing before you return from it. thanks, greg k-h
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
> > probe does not know what memory to allocate. We have several versions > > of the driver (with different init) and different versions allocate > > different state. > > I only see one driver here. It will be added in "PATCH v3 15/15". There will be two init functions allocating different states. > Why does probe not know what to allocate? That is exactly when the > device is bound to the driver, you have _way_ more information than you > do at init time. We have two versions of the driver. Probe asks for the version and calls the init function for this version. I don't want probe to know everything about all versions. > > >, not init time as what > > > happens if the hardware is not present yet your driver is loaded? > > > > init will have to rollback what it allocated. > > But those resources it will sit there wasted until unload happens. And > unload _never_ happens on a system unless you are a developer working on > the module. If probe fails I expect the kernel to release all resources. Is this not the case?
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
> > probe does not know what memory to allocate. We have several versions > > of the driver (with different init) and different versions allocate > > different state. > > I only see one driver here. It will be added in "PATCH v3 15/15". There will be two init functions allocating different states. > Why does probe not know what to allocate? That is exactly when the > device is bound to the driver, you have _way_ more information than you > do at init time. We have two versions of the driver. Probe asks for the version and calls the init function for this version. I don't want probe to know everything about all versions. > > >, not init time as what > > > happens if the hardware is not present yet your driver is loaded? > > > > init will have to rollback what it allocated. > > But those resources it will sit there wasted until unload happens. And > unload _never_ happens on a system unless you are a developer working on > the module. If probe fails I expect the kernel to release all resources. Is this not the case?
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
On Mon, Oct 15, 2018 at 11:48:28AM -0700, Roman Kiryanov wrote: > > You should only allocate memory at probe time > > probe does not know what memory to allocate. We have several versions > of the driver (with different init) and different versions allocate > different state. I only see one driver here. Why does probe not know what to allocate? That is exactly when the device is bound to the driver, you have _way_ more information than you do at init time. > >, not init time as what > > happens if the hardware is not present yet your driver is loaded? > > init will have to rollback what it allocated. But those resources it will sit there wasted until unload happens. And unload _never_ happens on a system unless you are a developer working on the module. thanks, greg k-h
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
On Mon, Oct 15, 2018 at 11:48:28AM -0700, Roman Kiryanov wrote: > > You should only allocate memory at probe time > > probe does not know what memory to allocate. We have several versions > of the driver (with different init) and different versions allocate > different state. I only see one driver here. Why does probe not know what to allocate? That is exactly when the device is bound to the driver, you have _way_ more information than you do at init time. > >, not init time as what > > happens if the hardware is not present yet your driver is loaded? > > init will have to rollback what it allocated. But those resources it will sit there wasted until unload happens. And unload _never_ happens on a system unless you are a developer working on the module. thanks, greg k-h
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
> You should only allocate memory at probe time probe does not know what memory to allocate. We have several versions of the driver (with different init) and different versions allocate different state. >, not init time as what > happens if the hardware is not present yet your driver is loaded? init will have to rollback what it allocated.
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
> You should only allocate memory at probe time probe does not know what memory to allocate. We have several versions of the driver (with different init) and different versions allocate different state. >, not init time as what > happens if the hardware is not present yet your driver is loaded? init will have to rollback what it allocated.
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
On Wed, Oct 03, 2018 at 10:17:11AM -0700, r...@google.com wrote: > From: Roman Kiryanov > > There will be two separate init functions for v1 and v2 > (different driver versions) and they will allocate different > state. You should only allocate memory at probe time, not init time as what happens if the hardware is not present yet your driver is loaded? You should do almost nothing at init time except register with the proper bus so that your probe function can be called if the hardware is present. So the patch here is going backwards from what it should be working toward. thanks, greg k-h
Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
On Wed, Oct 03, 2018 at 10:17:11AM -0700, r...@google.com wrote: > From: Roman Kiryanov > > There will be two separate init functions for v1 and v2 > (different driver versions) and they will allocate different > state. You should only allocate memory at probe time, not init time as what happens if the hardware is not present yet your driver is loaded? You should do almost nothing at init time except register with the proper bus so that your probe function can be called if the hardware is present. So the patch here is going backwards from what it should be working toward. thanks, greg k-h
[PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
From: Roman Kiryanov
There will be two separate init functions for v1 and v2
(different driver versions) and they will allocate different
state.
Signed-off-by: Roman Kiryanov
---
Changes in v3:
- No change.
Changes in v2:
- No change.
drivers/platform/goldfish/goldfish_pipe.c | 42 +--
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/platform/goldfish/goldfish_pipe.c
b/drivers/platform/goldfish/goldfish_pipe.c
index bc431f04c4cf..445c0c0c66c4 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -208,8 +208,6 @@ struct goldfish_pipe_dev {
struct device *pdev_dev;
/* Some device-specific data */
- int irq;
- int version;
unsigned char __iomem *base;
/* an irq tasklet to run goldfish_interrupt_task */
@@ -817,14 +815,23 @@ static void write_pa_addr(void *addr, void __iomem
*portl, void __iomem *porth)
}
static int goldfish_pipe_device_init(struct platform_device *pdev,
-struct goldfish_pipe_dev *dev)
+char __iomem *base,
+int irq)
{
+ struct goldfish_pipe_dev *dev;
int err;
+ dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ dev->magic = _pipe_device_deinit;
+ spin_lock_init(>lock);
+
tasklet_init(>irq_tasklet, _interrupt_task,
(unsigned long)dev);
- err = devm_request_irq(>dev, dev->irq,
+ err = devm_request_irq(>dev, irq,
goldfish_pipe_interrupt,
IRQF_SHARED, "goldfish_pipe", dev);
if (err) {
@@ -839,6 +846,7 @@ static int goldfish_pipe_device_init(struct platform_device
*pdev,
return err;
}
+ dev->base = base;
dev->pdev_dev = >dev;
dev->first_signalled_pipe = NULL;
dev->pipes_capacity = INITIAL_PIPES_CAPACITY;
@@ -892,22 +900,18 @@ static void goldfish_pipe_device_deinit(struct
platform_device *pdev,
static int goldfish_pipe_probe(struct platform_device *pdev)
{
struct resource *r;
- struct goldfish_pipe_dev *dev;
-
- dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
- if (!dev)
- return -ENOMEM;
-
- dev->magic = _pipe_device_deinit;
- spin_lock_init(>lock);
+ char __iomem *base;
+ int irq;
+ int version;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!r || resource_size(r) < PAGE_SIZE) {
dev_err(>dev, "can't allocate i/o page\n");
return -EINVAL;
}
- dev->base = devm_ioremap(>dev, r->start, PAGE_SIZE);
- if (!dev->base) {
+
+ base = devm_ioremap(>dev, r->start, PAGE_SIZE);
+ if (!base) {
dev_err(>dev, "ioremap failed\n");
return -EINVAL;
}
@@ -916,7 +920,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
if (!r)
return -EINVAL;
- dev->irq = r->start;
+ irq = r->start;
/*
* Exchange the versions with the host device
@@ -925,12 +929,12 @@ static int goldfish_pipe_probe(struct platform_device
*pdev)
* reading device version back: this allows the host implementation to
* detect the old driver (if there was no version write before read).
*/
- writel(PIPE_DRIVER_VERSION, dev->base + PIPE_REG_VERSION);
- dev->version = readl(dev->base + PIPE_REG_VERSION);
- if (WARN_ON(dev->version < PIPE_CURRENT_DEVICE_VERSION))
+ writel(PIPE_DRIVER_VERSION, base + PIPE_REG_VERSION);
+ version = readl(base + PIPE_REG_VERSION);
+ if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
return -EINVAL;
- return goldfish_pipe_device_init(pdev, dev);
+ return goldfish_pipe_device_init(pdev, base, irq);
}
static int goldfish_pipe_remove(struct platform_device *pdev)
--
2.19.0.605.g01d371f741-goog
[PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init
From: Roman Kiryanov
There will be two separate init functions for v1 and v2
(different driver versions) and they will allocate different
state.
Signed-off-by: Roman Kiryanov
---
Changes in v3:
- No change.
Changes in v2:
- No change.
drivers/platform/goldfish/goldfish_pipe.c | 42 +--
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/platform/goldfish/goldfish_pipe.c
b/drivers/platform/goldfish/goldfish_pipe.c
index bc431f04c4cf..445c0c0c66c4 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -208,8 +208,6 @@ struct goldfish_pipe_dev {
struct device *pdev_dev;
/* Some device-specific data */
- int irq;
- int version;
unsigned char __iomem *base;
/* an irq tasklet to run goldfish_interrupt_task */
@@ -817,14 +815,23 @@ static void write_pa_addr(void *addr, void __iomem
*portl, void __iomem *porth)
}
static int goldfish_pipe_device_init(struct platform_device *pdev,
-struct goldfish_pipe_dev *dev)
+char __iomem *base,
+int irq)
{
+ struct goldfish_pipe_dev *dev;
int err;
+ dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ dev->magic = _pipe_device_deinit;
+ spin_lock_init(>lock);
+
tasklet_init(>irq_tasklet, _interrupt_task,
(unsigned long)dev);
- err = devm_request_irq(>dev, dev->irq,
+ err = devm_request_irq(>dev, irq,
goldfish_pipe_interrupt,
IRQF_SHARED, "goldfish_pipe", dev);
if (err) {
@@ -839,6 +846,7 @@ static int goldfish_pipe_device_init(struct platform_device
*pdev,
return err;
}
+ dev->base = base;
dev->pdev_dev = >dev;
dev->first_signalled_pipe = NULL;
dev->pipes_capacity = INITIAL_PIPES_CAPACITY;
@@ -892,22 +900,18 @@ static void goldfish_pipe_device_deinit(struct
platform_device *pdev,
static int goldfish_pipe_probe(struct platform_device *pdev)
{
struct resource *r;
- struct goldfish_pipe_dev *dev;
-
- dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
- if (!dev)
- return -ENOMEM;
-
- dev->magic = _pipe_device_deinit;
- spin_lock_init(>lock);
+ char __iomem *base;
+ int irq;
+ int version;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!r || resource_size(r) < PAGE_SIZE) {
dev_err(>dev, "can't allocate i/o page\n");
return -EINVAL;
}
- dev->base = devm_ioremap(>dev, r->start, PAGE_SIZE);
- if (!dev->base) {
+
+ base = devm_ioremap(>dev, r->start, PAGE_SIZE);
+ if (!base) {
dev_err(>dev, "ioremap failed\n");
return -EINVAL;
}
@@ -916,7 +920,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
if (!r)
return -EINVAL;
- dev->irq = r->start;
+ irq = r->start;
/*
* Exchange the versions with the host device
@@ -925,12 +929,12 @@ static int goldfish_pipe_probe(struct platform_device
*pdev)
* reading device version back: this allows the host implementation to
* detect the old driver (if there was no version write before read).
*/
- writel(PIPE_DRIVER_VERSION, dev->base + PIPE_REG_VERSION);
- dev->version = readl(dev->base + PIPE_REG_VERSION);
- if (WARN_ON(dev->version < PIPE_CURRENT_DEVICE_VERSION))
+ writel(PIPE_DRIVER_VERSION, base + PIPE_REG_VERSION);
+ version = readl(base + PIPE_REG_VERSION);
+ if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
return -EINVAL;
- return goldfish_pipe_device_init(pdev, dev);
+ return goldfish_pipe_device_init(pdev, base, irq);
}
static int goldfish_pipe_remove(struct platform_device *pdev)
--
2.19.0.605.g01d371f741-goog
