[PATCH 1/2] i2c: pmcmsp: return message count on master_xfer success

[Peter Rosin]

Returning zero is wrong in this case.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/i2c/busses/i2c-pmcmsp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pmcmsp.c b/drivers/i2c/busses/i2c-pmcmsp.c
index 2aa0e83174c5..ec27e27e8d06 100644
--- a/drivers/i2c/busses/i2c-pmcmsp.c
+++ b/drivers/i2c/busses/i2c-pmcmsp.c
@@ -567,7 +567,7 @@ static int pmcmsptwi_master_xfer(struct i2c_adapter *adap,
return -1;
}
 
-   return 0;
+   return num;
 }
 
 static u32 pmcmsptwi_i2c_func(struct i2c_adapter *adapter)
-- 
2.11.0

[PATCH 1/2] i2c: pmcmsp: return message count on master_xfer success

[Peter Rosin]

Returning zero is wrong in this case.

Signed-off-by: Peter Rosin 
---
 drivers/i2c/busses/i2c-pmcmsp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pmcmsp.c b/drivers/i2c/busses/i2c-pmcmsp.c
index 2aa0e83174c5..ec27e27e8d06 100644
--- a/drivers/i2c/busses/i2c-pmcmsp.c
+++ b/drivers/i2c/busses/i2c-pmcmsp.c
@@ -567,7 +567,7 @@ static int pmcmsptwi_master_xfer(struct i2c_adapter *adap,
return -1;
}
 
-   return 0;
+   return num;
 }
 
 static u32 pmcmsptwi_i2c_func(struct i2c_adapter *adapter)
-- 
2.11.0

[PATCH 2/2] i2c: robotfuzz-osif: drop pointless test

[Peter Rosin]

In the for-loop test, ret will be either 0 or 1. So, the
comparison is pointless. Drop it, and drop the initializer
which is then also pointless.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/i2c/busses/i2c-robotfuzz-osif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-robotfuzz-osif.c 
b/drivers/i2c/busses/i2c-robotfuzz-osif.c
index 51d93b4b00f2..d848cf515234 100644
--- a/drivers/i2c/busses/i2c-robotfuzz-osif.c
+++ b/drivers/i2c/busses/i2c-robotfuzz-osif.c
@@ -62,10 +62,10 @@ static int osif_xfer(struct i2c_adapter *adapter, struct 
i2c_msg *msgs,
 {
struct osif_priv *priv = adapter->algo_data;
struct i2c_msg *pmsg;
-   int ret = 0;
+   int ret;
int i;
 
-   for (i = 0; ret >= 0 && i < num; i++) {
+   for (i = 0; i < num; i++) {
pmsg = [i];
 
if (pmsg->flags & I2C_M_RD) {
-- 
2.11.0

[PATCH 2/2] i2c: robotfuzz-osif: drop pointless test

[Peter Rosin]

In the for-loop test, ret will be either 0 or 1. So, the
comparison is pointless. Drop it, and drop the initializer
which is then also pointless.

Signed-off-by: Peter Rosin 
---
 drivers/i2c/busses/i2c-robotfuzz-osif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-robotfuzz-osif.c 
b/drivers/i2c/busses/i2c-robotfuzz-osif.c
index 51d93b4b00f2..d848cf515234 100644
--- a/drivers/i2c/busses/i2c-robotfuzz-osif.c
+++ b/drivers/i2c/busses/i2c-robotfuzz-osif.c
@@ -62,10 +62,10 @@ static int osif_xfer(struct i2c_adapter *adapter, struct 
i2c_msg *msgs,
 {
struct osif_priv *priv = adapter->algo_data;
struct i2c_msg *pmsg;
-   int ret = 0;
+   int ret;
int i;
 
-   for (i = 0; ret >= 0 && i < num; i++) {
+   for (i = 0; i < num; i++) {
pmsg = [i];
 
if (pmsg->flags & I2C_M_RD) {
-- 
2.11.0

[PATCH 2/2] i2c: exynos5: remove pointless initializers

[Peter Rosin]

The variables are always assigned before use anyway.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/i2c/busses/i2c-exynos5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index a2cbc779c33a..185fba37e830 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -707,7 +707,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
 {
struct exynos5_i2c *i2c = adap->algo_data;
-   int i = 0, ret = 0, stop = 0;
+   int i, ret, stop;
 
if (i2c->suspended) {
dev_err(i2c->dev, "HS-I2C is not initialized.\n");
-- 
2.11.0

[PATCH 2/2] i2c: exynos5: remove pointless initializers

[Peter Rosin]

The variables are always assigned before use anyway.

Signed-off-by: Peter Rosin 
---
 drivers/i2c/busses/i2c-exynos5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index a2cbc779c33a..185fba37e830 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -707,7 +707,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
 {
struct exynos5_i2c *i2c = adap->algo_data;
-   int i = 0, ret = 0, stop = 0;
+   int i, ret, stop;
 
if (i2c->suspended) {
dev_err(i2c->dev, "HS-I2C is not initialized.\n");
-- 
2.11.0

[PATCH] i2c: hix5hd2: remove some dead code

[Peter Rosin]

The else branch cannot be taken as i will always equal num.
Get rid of the whole construct.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/i2c/busses/i2c-hix5hd2.c | 12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-hix5hd2.c b/drivers/i2c/busses/i2c-hix5hd2.c
index bb68957d3da5..190b250e6834 100644
--- a/drivers/i2c/busses/i2c-hix5hd2.c
+++ b/drivers/i2c/busses/i2c-hix5hd2.c
@@ -377,17 +377,7 @@ static int hix5hd2_i2c_xfer(struct i2c_adapter *adap,
goto out;
}
 
-   if (i == num) {
-   ret = num;
-   } else {
-   /* Only one message, cannot access the device */
-   if (i == 1)
-   ret = -EREMOTEIO;
-   else
-   ret = i;
-
-   dev_warn(priv->dev, "xfer message failed\n");
-   }
+   ret = num;
 
 out:
pm_runtime_mark_last_busy(priv->dev);
-- 
2.11.0

[PATCH] i2c: hix5hd2: remove some dead code

[Peter Rosin]

The else branch cannot be taken as i will always equal num.
Get rid of the whole construct.

Signed-off-by: Peter Rosin 
---
 drivers/i2c/busses/i2c-hix5hd2.c | 12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-hix5hd2.c b/drivers/i2c/busses/i2c-hix5hd2.c
index bb68957d3da5..190b250e6834 100644
--- a/drivers/i2c/busses/i2c-hix5hd2.c
+++ b/drivers/i2c/busses/i2c-hix5hd2.c
@@ -377,17 +377,7 @@ static int hix5hd2_i2c_xfer(struct i2c_adapter *adap,
goto out;
}
 
-   if (i == num) {
-   ret = num;
-   } else {
-   /* Only one message, cannot access the device */
-   if (i == 1)
-   ret = -EREMOTEIO;
-   else
-   ret = i;
-
-   dev_warn(priv->dev, "xfer message failed\n");
-   }
+   ret = num;
 
 out:
pm_runtime_mark_last_busy(priv->dev);
-- 
2.11.0

[PATCH 1/2] i2c: exynos5: remove some dead code

[Peter Rosin]

The else branch cannot be taken as i will always equal num.
Get rid of the whole construct.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/i2c/busses/i2c-exynos5.c | 12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 12ec8484e653..a2cbc779c33a 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -727,17 +727,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
goto out;
}
 
-   if (i == num) {
-   ret = num;
-   } else {
-   /* Only one message, cannot access the device */
-   if (i == 1)
-   ret = -EREMOTEIO;
-   else
-   ret = i;
-
-   dev_warn(i2c->dev, "xfer message failed\n");
-   }
+   ret = num;
 
  out:
clk_disable(i2c->clk);
-- 
2.11.0

[PATCH 1/2] i2c: exynos5: remove some dead code

[Peter Rosin]

The else branch cannot be taken as i will always equal num.
Get rid of the whole construct.

Signed-off-by: Peter Rosin 
---
 drivers/i2c/busses/i2c-exynos5.c | 12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 12ec8484e653..a2cbc779c33a 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -727,17 +727,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
goto out;
}
 
-   if (i == num) {
-   ret = num;
-   } else {
-   /* Only one message, cannot access the device */
-   if (i == 1)
-   ret = -EREMOTEIO;
-   else
-   ret = i;
-
-   dev_warn(i2c->dev, "xfer message failed\n");
-   }
+   ret = num;
 
  out:
clk_disable(i2c->clk);
-- 
2.11.0

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

On 2018-05-09 17:08, Andrzej Hajda wrote:
> On 04.05.2018 15:51, Peter Rosin wrote:
>> Bridge drivers can now (temporarily, in a transition phase) select if
>> they want to provide a full owner device or keep just providing an
>> of_node.
>>
>> By providing a full owner device, the bridge drivers no longer need
>> to provide an of_node since that node is available via the owner
>> device.
>>
>> When all bridge drivers provide an owner device, that will become
>> mandatory and the .of_node member will be removed.
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 3 ++-
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
> 
> What is the reason to put rockchip here? Shouldn't be in separate patch?

Because the rockchip driver peeks into the bridge struct and all the
changes in this patch relate to making the new .odev member optional in
the transition phase, when the bridge can have either a new-style odev
or an old style of_node.

I guess this rockchip change could be patch 2, but it has to come first
after this patch and it makes no sense on its own. Hence, one patch.

This rockchip_lvds interaction is also present in patch 24/26
drm/bridge: remove the .of_node member

I can split them if you want, but I personally don't see the point.

>>  include/drm/drm_bridge.h | 2 ++
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 1638bfe9627c..3872f5379998 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
>> *np)
>>  mutex_lock(_lock);
>>  
>>  list_for_each_entry(bridge, _list, list) {
>> -if (bridge->of_node == np) {
>> +if ((bridge->odev && bridge->odev->of_node == np) ||
>> +bridge->of_node == np) {
>>  mutex_unlock(_lock);
>>  return bridge;
>>  }
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
>> b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> index 4bd94b167d2c..557e0079c98d 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> @@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, 
>> struct device *master,
>>  }
>>  if (lvds->panel)
>>  remote = lvds->panel->dev->of_node;
>> -else
>> +else if (lvds->bridge->of_node)
>>  remote = lvds->bridge->of_node;
>> +else
>> +remote = lvds->bridge->odev->of_node;
> 
> I guess odev should be NULL here, or have I missed something.

s/should/could/ ???

Assuming that was what you meant and answering accordingly...

No, .odev cannot be NULL in that else branch. drm_of_find_panel_or_bridge
either found a panel or a bridge (or it failed). If it found a bridge
(which is the relevant branch for this question) that bridge would have
to have either an of_node (in the transition phase) or a valid .odev.
Otherwise the bridge could not have been found by
drm_of_find_panel_or_bridge.

*time passes*

Ahh, yes, .odev is always NULL here so you probably did mean "should".
But after patches 2-23 when bridges start populating .odev *instead*
of .of_node, .odev will not remain NULL. But as I said above, while
.odev is NULL, .of_node will never be NULL and vice versa (or
drm_of_find_panel_or_bridge could not have found the bridge) so there
is no risk of a NULL dereference.

Cheers,
Peter

> 
> Regards
> Andrzej
> 
>>  if (of_property_read_string(dev->of_node, "rockchip,output", ))
>>  /* default set it as output rgb */
>>  lvds->output = DISPLAY_OUTPUT_RGB;
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..7c17977c3537 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -254,6 +254,7 @@ struct drm_bridge_timings {
>>  
>>  /**
>>   * struct drm_bridge - central DRM bridge control structure
>> + * @odev: device that owns the bridge
>>   * @dev: DRM device this bridge belongs to
>>   * @encoder: encoder to which this bridge is connected
>>   * @next: the next bridge in the encoder chain
>> @@ -265,6 +266,7 @@ struct drm_bridge_timings {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> +struct device *odev;
>>  struct drm_device *dev;
>>  struct drm_encoder *encoder;
>>  struct drm_bridge *next;
> 
> 

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

On 2018-05-09 17:08, Andrzej Hajda wrote:
> On 04.05.2018 15:51, Peter Rosin wrote:
>> Bridge drivers can now (temporarily, in a transition phase) select if
>> they want to provide a full owner device or keep just providing an
>> of_node.
>>
>> By providing a full owner device, the bridge drivers no longer need
>> to provide an of_node since that node is available via the owner
>> device.
>>
>> When all bridge drivers provide an owner device, that will become
>> mandatory and the .of_node member will be removed.
>>
>> Signed-off-by: Peter Rosin 
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 3 ++-
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
> 
> What is the reason to put rockchip here? Shouldn't be in separate patch?

Because the rockchip driver peeks into the bridge struct and all the
changes in this patch relate to making the new .odev member optional in
the transition phase, when the bridge can have either a new-style odev
or an old style of_node.

I guess this rockchip change could be patch 2, but it has to come first
after this patch and it makes no sense on its own. Hence, one patch.

This rockchip_lvds interaction is also present in patch 24/26
drm/bridge: remove the .of_node member

I can split them if you want, but I personally don't see the point.

>>  include/drm/drm_bridge.h | 2 ++
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 1638bfe9627c..3872f5379998 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
>> *np)
>>  mutex_lock(_lock);
>>  
>>  list_for_each_entry(bridge, _list, list) {
>> -if (bridge->of_node == np) {
>> +if ((bridge->odev && bridge->odev->of_node == np) ||
>> +bridge->of_node == np) {
>>  mutex_unlock(_lock);
>>  return bridge;
>>  }
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
>> b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> index 4bd94b167d2c..557e0079c98d 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> @@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, 
>> struct device *master,
>>  }
>>  if (lvds->panel)
>>  remote = lvds->panel->dev->of_node;
>> -else
>> +else if (lvds->bridge->of_node)
>>  remote = lvds->bridge->of_node;
>> +else
>> +remote = lvds->bridge->odev->of_node;
> 
> I guess odev should be NULL here, or have I missed something.

s/should/could/ ???

Assuming that was what you meant and answering accordingly...

No, .odev cannot be NULL in that else branch. drm_of_find_panel_or_bridge
either found a panel or a bridge (or it failed). If it found a bridge
(which is the relevant branch for this question) that bridge would have
to have either an of_node (in the transition phase) or a valid .odev.
Otherwise the bridge could not have been found by
drm_of_find_panel_or_bridge.

*time passes*

Ahh, yes, .odev is always NULL here so you probably did mean "should".
But after patches 2-23 when bridges start populating .odev *instead*
of .of_node, .odev will not remain NULL. But as I said above, while
.odev is NULL, .of_node will never be NULL and vice versa (or
drm_of_find_panel_or_bridge could not have found the bridge) so there
is no risk of a NULL dereference.

Cheers,
Peter

> 
> Regards
> Andrzej
> 
>>  if (of_property_read_string(dev->of_node, "rockchip,output", ))
>>  /* default set it as output rgb */
>>  lvds->output = DISPLAY_OUTPUT_RGB;
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..7c17977c3537 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -254,6 +254,7 @@ struct drm_bridge_timings {
>>  
>>  /**
>>   * struct drm_bridge - central DRM bridge control structure
>> + * @odev: device that owns the bridge
>>   * @dev: DRM device this bridge belongs to
>>   * @encoder: encoder to which this bridge is connected
>>   * @next: the next bridge in the encoder chain
>> @@ -265,6 +266,7 @@ struct drm_bridge_timings {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> +struct device *odev;
>>  struct drm_device *dev;
>>  struct drm_encoder *encoder;
>>  struct drm_bridge *next;
> 
> 

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Peter Rosin]

On 2018-05-08 18:32, Wolfram Sang wrote:
> Grygorii,
> 
> thanks a lot for your input. Much appreciated!
> 
>> That would be great, but note:
>> 1) only i2c_transfer() operations are locked, so if driver is doing 
>> i2c_transfer(1)
>> i2c_transfer(2) <- suspend in the middle
>> <- suspend in between
>> i2c_transfer(3)
>> It will not help.
> 
> Will it not improve the situation by ensuring that at least the transfer
> with its (potenitally) multiple messages got completed? That we are at
> least in a bus-free state (assuming single-master here) before
> suspending?
> 
>> Everything depends on timings :( - in my practice 1 suspend iteration 
>> tests
>> where required to run many times to catch 3 buggy I2C client drivers.
> 
> Matches my experiences that creating a reliable test case for that is
> not that easy as I thought. Or I am missing something obvious.
> 
>> 2) It's normal to abort suspend if system is busy, so if I2C core will be 
>> able
>> to catch active I2C operation - it should abort, but again I do not see how 
>> it
>>  can be detected 100% with current I2C core design or without reworking huge 
>> number of drivers.
> 
> I agree. After second thought, waiting for i2c_transfer to finish maybe
> won't be enough, I am afraid. We don't know if STOP has been put on the
> wires yet. My best bet now is that we implement such a
> 'is-transfer-ongoing'-check in the suspend function of the master
> driver? That check should be optional, but recommended.
> 
>> 3) So, only one thing I2C core potentially can do - catch invalid access and
>> report it. "wait for transfer to finish" wouldn't work as for me.
> 
> And we do this in suspend_noirq function of the i2c core.
> 
>>> I at least know of some Renesas boards which needed the I2C connected
>>> PMIC to do a system reset (not sure about suspend, need to recheck
>>> that). That still today causes problems because interrupts are disabled
>>> then.
>>
>> this was triggered few times already (sry, don't have links), as of now,
>>  and as I know, the only ways to W/A this is:
>> - to create barametal platform driver (some time in ASM)
>> - or delegate final suspend operation to another system controller 
>> (co-processor),
>>   as example TI am335x SoCs,
>> - or implement I2C driver in hw - TI AVS/SmartReflex. 
> 
> Yes. Please note that this is only needed for reset, not suspend. So, it
> is a bit easier. Still, it might make more sense to use a platform based
> solution. I'll think about that.
> 
>> Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
>> .suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.
> 
> I do believe you, still is there documentation about such things? I like
> to understand more but didn't dig up something up to now. E.g. I grepped
> for "noirq" in Documentation/power.
> 
>> "master_xfer_irqless" might be a not bad idea, but, in my opinion, it 
>> should be used explicitly by platform code only, and each usage should
>> be proved to exist. 
> 
> Yes, we can think about it once it is really needed.
> 
>> Some additional info:
> 
> Thanks a lot for that!
> 
>> I'm attaching some very old patch (don't ask me why it was not sent :()
>> I did for Android system - which likes suspend very much. Some
>> part of below diff are obsolete now (like omap_i2c_suspend()),
>> but .noirq() callback are still valid and can show over all idea.
>> Really helped to catch min 3 buggy client drivers with timers, delayed
>> or periodic works.
> 
> Ok, so what do you think about my plan to:
> 
> 1) encourage drivers to check if there is still an ongoing transfer in
> their .suspend function (or the core can do that, too, if we agree that
> checking for a taken adapter lock is sufficient)
> 
> -> to ensure transfers don't get interrupted in the middle

A note from the peanut gallery: the adapter lock is not sufficient when
there are mux-locked muxes on the bus.

Cheers,
Peter

> 2) use a .suspend_noirq callback in i2c_bus_type.pm to reject and WARN
> about transfers still going on in that phase
> 
> -> this ensures that buggy drivers are caught
> 
> 3) write some documentation about our findings / assumptions /
> recommendations to a file in Documentation/i2c/
> 
> -> this ensures we won't forget why we did things like they are ;)
> 
> ?
> 
> Kind regards,
> 
>Wolfram
> 

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Peter Rosin]

On 2018-05-08 18:32, Wolfram Sang wrote:
> Grygorii,
> 
> thanks a lot for your input. Much appreciated!
> 
>> That would be great, but note:
>> 1) only i2c_transfer() operations are locked, so if driver is doing 
>> i2c_transfer(1)
>> i2c_transfer(2) <- suspend in the middle
>> <- suspend in between
>> i2c_transfer(3)
>> It will not help.
> 
> Will it not improve the situation by ensuring that at least the transfer
> with its (potenitally) multiple messages got completed? That we are at
> least in a bus-free state (assuming single-master here) before
> suspending?
> 
>> Everything depends on timings :( - in my practice 1 suspend iteration 
>> tests
>> where required to run many times to catch 3 buggy I2C client drivers.
> 
> Matches my experiences that creating a reliable test case for that is
> not that easy as I thought. Or I am missing something obvious.
> 
>> 2) It's normal to abort suspend if system is busy, so if I2C core will be 
>> able
>> to catch active I2C operation - it should abort, but again I do not see how 
>> it
>>  can be detected 100% with current I2C core design or without reworking huge 
>> number of drivers.
> 
> I agree. After second thought, waiting for i2c_transfer to finish maybe
> won't be enough, I am afraid. We don't know if STOP has been put on the
> wires yet. My best bet now is that we implement such a
> 'is-transfer-ongoing'-check in the suspend function of the master
> driver? That check should be optional, but recommended.
> 
>> 3) So, only one thing I2C core potentially can do - catch invalid access and
>> report it. "wait for transfer to finish" wouldn't work as for me.
> 
> And we do this in suspend_noirq function of the i2c core.
> 
>>> I at least know of some Renesas boards which needed the I2C connected
>>> PMIC to do a system reset (not sure about suspend, need to recheck
>>> that). That still today causes problems because interrupts are disabled
>>> then.
>>
>> this was triggered few times already (sry, don't have links), as of now,
>>  and as I know, the only ways to W/A this is:
>> - to create barametal platform driver (some time in ASM)
>> - or delegate final suspend operation to another system controller 
>> (co-processor),
>>   as example TI am335x SoCs,
>> - or implement I2C driver in hw - TI AVS/SmartReflex. 
> 
> Yes. Please note that this is only needed for reset, not suspend. So, it
> is a bit easier. Still, it might make more sense to use a platform based
> solution. I'll think about that.
> 
>> Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
>> .suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.
> 
> I do believe you, still is there documentation about such things? I like
> to understand more but didn't dig up something up to now. E.g. I grepped
> for "noirq" in Documentation/power.
> 
>> "master_xfer_irqless" might be a not bad idea, but, in my opinion, it 
>> should be used explicitly by platform code only, and each usage should
>> be proved to exist. 
> 
> Yes, we can think about it once it is really needed.
> 
>> Some additional info:
> 
> Thanks a lot for that!
> 
>> I'm attaching some very old patch (don't ask me why it was not sent :()
>> I did for Android system - which likes suspend very much. Some
>> part of below diff are obsolete now (like omap_i2c_suspend()),
>> but .noirq() callback are still valid and can show over all idea.
>> Really helped to catch min 3 buggy client drivers with timers, delayed
>> or periodic works.
> 
> Ok, so what do you think about my plan to:
> 
> 1) encourage drivers to check if there is still an ongoing transfer in
> their .suspend function (or the core can do that, too, if we agree that
> checking for a taken adapter lock is sufficient)
> 
> -> to ensure transfers don't get interrupted in the middle

A note from the peanut gallery: the adapter lock is not sufficient when
there are mux-locked muxes on the bus.

Cheers,
Peter

> 2) use a .suspend_noirq callback in i2c_bus_type.pm to reject and WARN
> about transfers still going on in that phase
> 
> -> this ensures that buggy drivers are caught
> 
> 3) write some documentation about our findings / assumptions /
> recommendations to a file in Documentation/i2c/
> 
> -> this ensures we won't forget why we did things like they are ;)
> 
> ?
> 
> Kind regards,
> 
>Wolfram
> 

Re: [PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Peter Rosin]

On 2018-05-08 08:51, Jyri Sarha wrote:
> On 05/04/18 16:51, Peter Rosin wrote:
>> It gets rid of an #ifdef and the .of_node member is going away.
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>  drivers/gpu/drm/bridge/panel.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
>> index 6d99d4a3beb3..f43d77b5ed20 100644
>> --- a/drivers/gpu/drm/bridge/panel.c
>> +++ b/drivers/gpu/drm/bridge/panel.c
>> @@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct 
>> drm_panel *panel,
>>  panel_bridge->connector_type = connector_type;
>>  panel_bridge->panel = panel;
>>  
>> +panel_bridge->bridge.odev = panel->dev;
> 
> I am afraid this approach will eventually conflict with my lately
> accepted patch[1].

I don't see how? The links are refcounted. So, if there is one link
each for the panel and bridge between the drm device and the panel
device that link will simply get two references. If/when the panel
device then goes away, the drm device will be brought down because
of that link (with two references, but that is irrelevant). When
the drm device is brought down, it will (presumably) bring down the
bridge as well (which will fix the refcount as the bridge link is
killed as part of that).

Or have you done some test and seen an actual problem?

> The panel already has a device pointer of its own, and technically the
> bridge element created here is created by the master drm device itself.

Not always, some bridges also call drm_panel_bridge_add for connecting
its output to either a panel or another bridge.

And by that line of argument, the devm_kzalloc in drm_panel_bridge_add
attaches the bridge memory to the wrong device as well. Maybe that
should be fixed? Seems orthogonal though, but it would introduce a
natural struct device in that function to assign to .odev. I think
the device owning the bridge memory should be the same as the .odev
device of the bridge.

> I suggest assigning odev here to NULL or to master drm device itself.

I'd rather not use NULL, since it is nice to be able to rely on the
.odev being there, and WARN if it isn't.

Cheers,
Peter

> Best regards,
> Jyri
> 
>>  panel_bridge->bridge.funcs = _bridge_bridge_funcs;
>> -#ifdef CONFIG_OF
>> -panel_bridge->bridge.of_node = panel->dev->of_node;
>> -#endif
>>  
>>  drm_bridge_add(_bridge->bridge);
>>  
>>
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/174559.html
> 

Re: [PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Peter Rosin]

On 2018-05-08 08:51, Jyri Sarha wrote:
> On 05/04/18 16:51, Peter Rosin wrote:
>> It gets rid of an #ifdef and the .of_node member is going away.
>>
>> Signed-off-by: Peter Rosin 
>> ---
>>  drivers/gpu/drm/bridge/panel.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
>> index 6d99d4a3beb3..f43d77b5ed20 100644
>> --- a/drivers/gpu/drm/bridge/panel.c
>> +++ b/drivers/gpu/drm/bridge/panel.c
>> @@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct 
>> drm_panel *panel,
>>  panel_bridge->connector_type = connector_type;
>>  panel_bridge->panel = panel;
>>  
>> +panel_bridge->bridge.odev = panel->dev;
> 
> I am afraid this approach will eventually conflict with my lately
> accepted patch[1].

I don't see how? The links are refcounted. So, if there is one link
each for the panel and bridge between the drm device and the panel
device that link will simply get two references. If/when the panel
device then goes away, the drm device will be brought down because
of that link (with two references, but that is irrelevant). When
the drm device is brought down, it will (presumably) bring down the
bridge as well (which will fix the refcount as the bridge link is
killed as part of that).

Or have you done some test and seen an actual problem?

> The panel already has a device pointer of its own, and technically the
> bridge element created here is created by the master drm device itself.

Not always, some bridges also call drm_panel_bridge_add for connecting
its output to either a panel or another bridge.

And by that line of argument, the devm_kzalloc in drm_panel_bridge_add
attaches the bridge memory to the wrong device as well. Maybe that
should be fixed? Seems orthogonal though, but it would introduce a
natural struct device in that function to assign to .odev. I think
the device owning the bridge memory should be the same as the .odev
device of the bridge.

> I suggest assigning odev here to NULL or to master drm device itself.

I'd rather not use NULL, since it is nice to be able to rely on the
.odev being there, and WARN if it isn't.

Cheers,
Peter

> Best regards,
> Jyri
> 
>>  panel_bridge->bridge.funcs = _bridge_bridge_funcs;
>> -#ifdef CONFIG_OF
>> -panel_bridge->bridge.of_node = panel->dev->of_node;
>> -#endif
>>  
>>  drm_bridge_add(_bridge->bridge);
>>  
>>
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/174559.html
> 

Re: [PATCH] i2c-mux-pca954x: Force reset on probe if available

[Peter Rosin]

On 2018-05-01 13:42, Mike Looijmans wrote:
> Instead of just hogging the reset GPIO into deactivated state, activate and
> then de-activate the reset. This allows for better recovery if the CPU was
> reset halfway through an I2C transaction for example.

I can't see any problems with this, and a reset at load time can certainly
be a benefit. Some questions below though...

Cheers,
Peter

> Signed-off-by: Mike Looijmans 
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c 
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 09bafd3..13e10d0 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -36,6 +36,7 @@
>   */
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -389,10 +390,17 @@ static int pca954x_probe(struct i2c_client *client,
>   i2c_set_clientdata(client, muxc);
>   data->client = client;
>  
> - /* Get the mux out of reset if a reset GPIO is specified. */
> - gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
> + /* Reset the mux if a reset GPIO is specified. */
> + gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_HIGH);
>   if (IS_ERR(gpio))
>   return PTR_ERR(gpio);
> + if (gpio) {
> + /* Datasheet specifies a 4 ns reset-low time */
> + udelay(1);

ndelay(4) ?

> + gpiod_set_value_cansleep(gpio, 0);
> + /* Datasheet specifies a 500 ns reset recovery time */
> + udelay(1);

ndelay(500) ?

> + }
>  
>   match = of_match_device(of_match_ptr(pca954x_of_match), >dev);
>   if (match)
> 

Re: [PATCH] i2c-mux-pca954x: Force reset on probe if available

[Peter Rosin]

On 2018-05-01 13:42, Mike Looijmans wrote:
> Instead of just hogging the reset GPIO into deactivated state, activate and
> then de-activate the reset. This allows for better recovery if the CPU was
> reset halfway through an I2C transaction for example.

I can't see any problems with this, and a reset at load time can certainly
be a benefit. Some questions below though...

Cheers,
Peter

> Signed-off-by: Mike Looijmans 
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c 
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 09bafd3..13e10d0 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -36,6 +36,7 @@
>   */
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -389,10 +390,17 @@ static int pca954x_probe(struct i2c_client *client,
>   i2c_set_clientdata(client, muxc);
>   data->client = client;
>  
> - /* Get the mux out of reset if a reset GPIO is specified. */
> - gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
> + /* Reset the mux if a reset GPIO is specified. */
> + gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_HIGH);
>   if (IS_ERR(gpio))
>   return PTR_ERR(gpio);
> + if (gpio) {
> + /* Datasheet specifies a 4 ns reset-low time */
> + udelay(1);

ndelay(4) ?

> + gpiod_set_value_cansleep(gpio, 0);
> + /* Datasheet specifies a 500 ns reset recovery time */
> + udelay(1);

ndelay(500) ?

> + }
>  
>   match = of_match_device(of_match_ptr(pca954x_of_match), >dev);
>   if (match)
> 

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:59, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
>> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>>> but there are several other bridges that never call drm_bridge_add.
>>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>>
>>>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>>>
>>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>>
>>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>>   drm driver to connect/disconnect a drm_bridge.
>>>>
>>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>>   driver itself to register/unregister itself. Maybe we should rename
>>>>   them, since the same issue happens with drm_panel, with the same
>>>>   confusion.
>>>>
>>>> I thought someone was working on a cleanup series to fix this mess, but I
>>>> didn't find anything.
>>>
>>> Ok, I just spotted the imbalance and didn't really dig into what
>>> actually happens in these error paths. Now that I have done so I
>>> believe that the removed drm_bridge_remove calls causes NULL
>>> dereferences if/when the error paths are triggered.
>>>
>>> So, I don't think this can wait for some bigger cleanup.
>>>
>>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>>> __list_del with NULL in both prev and next since the list member
>>> is never initialized. prev and next are dereferenced by __list_del
>>> and you have *boom*
>>>
>>> I recommend adding the tag
>>>
>>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>>
>>> so that stable picks this one up.
>>
>> I just wanted to correct your commit message text - the correct solution
>> is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?

Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
internal use. It does not drm_bridge_add it, because all that ever does
is adding the bridge to the global lost of bridges. But since this is
a bridge for internal use, there is little point in calling drm_bridge_add,
the driver currently gains nothing by doing so.

But, drm_bridge_add might be a good place to put common stuff for every
bridge in the system, so it might be worthwhile to start requiring all
bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
the sti-hda driver call drm_bridge_add on the bridge it creates.

Do you really think it is actively wrong to call drm_bridge_add for
internal bridges such as this?

Cheers,
Peter

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:59, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
>> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>>> but there are several other bridges that never call drm_bridge_add.
>>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>>
>>>>> Signed-off-by: Peter Rosin 
>>>>
>>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>>
>>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>>   drm driver to connect/disconnect a drm_bridge.
>>>>
>>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>>   driver itself to register/unregister itself. Maybe we should rename
>>>>   them, since the same issue happens with drm_panel, with the same
>>>>   confusion.
>>>>
>>>> I thought someone was working on a cleanup series to fix this mess, but I
>>>> didn't find anything.
>>>
>>> Ok, I just spotted the imbalance and didn't really dig into what
>>> actually happens in these error paths. Now that I have done so I
>>> believe that the removed drm_bridge_remove calls causes NULL
>>> dereferences if/when the error paths are triggered.
>>>
>>> So, I don't think this can wait for some bigger cleanup.
>>>
>>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>>> __list_del with NULL in both prev and next since the list member
>>> is never initialized. prev and next are dereferenced by __list_del
>>> and you have *boom*
>>>
>>> I recommend adding the tag
>>>
>>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>>
>>> so that stable picks this one up.
>>
>> I just wanted to correct your commit message text - the correct solution
>> is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?

Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
internal use. It does not drm_bridge_add it, because all that ever does
is adding the bridge to the global lost of bridges. But since this is
a bridge for internal use, there is little point in calling drm_bridge_add,
the driver currently gains nothing by doing so.

But, drm_bridge_add might be a good place to put common stuff for every
bridge in the system, so it might be worthwhile to start requiring all
bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
the sti-hda driver call drm_bridge_add on the bridge it creates.

Do you really think it is actively wrong to call drm_bridge_add for
internal bridges such as this?

Cheers,
Peter

Re: [PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-05-07 15:56, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 03:51:46PM +0200, Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 25 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. Jyri, are
>> you able to test? That said, I might have missed some subtlety.
>>
>> Oh and the reason I'm pushing this is of course so that the issue
>> noted by Russel in [1] is addressed which in turn means that the
>> tda998x bridge driver can be patched according to that series without
>> objection (hopefully) and then used from the atmel-hlcdc driver (and
>> other drivers that are not componentized).
>>
>> Changes since v1https://lkml.org/lkml/2018/4/26/1018
>>
>> - rename .owner to .odev to not get mixed up with the module owner.
>> - added patches for new recent drivers thc63lvd1024 and cdns-dsi
>> - fix for problem in the rockchip_lvds driver reported by 0day
>> - added a WARN in drm_bridge_add if there is no .odev owner device
>>
>> I did *not*:
>> - add any ack from Daniel since he suggested "pdev", and I ended up
>>   with "odev" in the rename since I disliked "pdev" about as much
>>   as "owner".
> 
> As long as it's not owner, I'm fine :-) Ack on the idea still holds.
> 
>> - add any port id. The current .of_node (that this series removes)
>>   does not identify the port, so that problem seems orthogonal
>>   to me.
> 
> Hm, from my cursory DT/of code reading last week I thought the port is
> used to lookup the right node, but there's no port thing on the target for
> a phandle? At least that's how current drm_of_find_panel_or_bridge seems
> to work ...

drm_of_find_panel_or_bridge calls of_graph_get_remote_node and that
function looks up the main remote device node, i.e. not the remote
port or endpoint node but the parent node. So, bridges using .of_node
have stored their main device node in the .of_node member. I.e. the
same value as of_node in struct device for all current cases.

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>> [1] https://lkml.org/lkml/2018/4/23/769
>> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
>>
>> Peter Rosin (26):
>>   drm/bridge: allow optionally specifying an owner .odev device
>>   drm/bridge: adv7511: provide an owner .odev device
>>   drm/bridge/analogix: core: specify the owner .odev of the bridge
>>   drm/bridge: analogix-anx78xx: provide an owner .odev device
>>   drm/bridge: cdns-dsi: provide an owner .odev device
>>   drm/bridge: vga-dac: provide an owner .odev device
>>   drm/bridge: lvds-encoder: provide an owner .odev device
>>   drm/bridge: megachips-stdp-ge-b850v3-fw: provide an owner .odev
>> device
>>   drm/bridge: nxp-ptn3460: provide an owner .odev device
>>   drm/bridge: panel: provide an owner .odev device
>>   drm/bridge: ps8622: provide an owner .odev device
>>   drm/bridge: sii902x: provide an owner .odev device
>>   drm/bridge: sii9234: provide an owner .odev device
>>   drm/bridge: sii8620: provide an owner .odev device
>>   drm/bridge: synopsys: provide an owner .odev device for the bridges
>>   drm/bridge: tc358767: provide an owner .odev device
>>   drm/bridge: thc63lvd1024: provide an owner .odev device
>>   drm/bridge: ti-tfp410: provide an owner .odev device
>>   drm/exynos: mic: provide an owner .odev device for the bridge
>>   drm/mediatek: hdmi: provide an owner .odev device for the bridge
>>   drm/msm: specify the owner .odev of the bridges
>>   drm/rcar-du: lvds: provide an owner .odev device for the bridge
>>   drm/sti: provide an owner .odev device for the bridges
>>   drm/bridge: remove the .of_node member
>>   drm/bridge: require the owner .odev to be filled in on
>> drm_bridge_add/attach
>>   drm/bridge: establish a link between the bridge supplier and consumer
>>
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
>>  drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
>>  drivers/gpu/drm/bridge/cdns-dsi.c  |  

Re: [PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-05-07 15:56, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 03:51:46PM +0200, Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 25 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. Jyri, are
>> you able to test? That said, I might have missed some subtlety.
>>
>> Oh and the reason I'm pushing this is of course so that the issue
>> noted by Russel in [1] is addressed which in turn means that the
>> tda998x bridge driver can be patched according to that series without
>> objection (hopefully) and then used from the atmel-hlcdc driver (and
>> other drivers that are not componentized).
>>
>> Changes since v1https://lkml.org/lkml/2018/4/26/1018
>>
>> - rename .owner to .odev to not get mixed up with the module owner.
>> - added patches for new recent drivers thc63lvd1024 and cdns-dsi
>> - fix for problem in the rockchip_lvds driver reported by 0day
>> - added a WARN in drm_bridge_add if there is no .odev owner device
>>
>> I did *not*:
>> - add any ack from Daniel since he suggested "pdev", and I ended up
>>   with "odev" in the rename since I disliked "pdev" about as much
>>   as "owner".
> 
> As long as it's not owner, I'm fine :-) Ack on the idea still holds.
> 
>> - add any port id. The current .of_node (that this series removes)
>>   does not identify the port, so that problem seems orthogonal
>>   to me.
> 
> Hm, from my cursory DT/of code reading last week I thought the port is
> used to lookup the right node, but there's no port thing on the target for
> a phandle? At least that's how current drm_of_find_panel_or_bridge seems
> to work ...

drm_of_find_panel_or_bridge calls of_graph_get_remote_node and that
function looks up the main remote device node, i.e. not the remote
port or endpoint node but the parent node. So, bridges using .of_node
have stored their main device node in the .of_node member. I.e. the
same value as of_node in struct device for all current cases.

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>> [1] https://lkml.org/lkml/2018/4/23/769
>> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
>>
>> Peter Rosin (26):
>>   drm/bridge: allow optionally specifying an owner .odev device
>>   drm/bridge: adv7511: provide an owner .odev device
>>   drm/bridge/analogix: core: specify the owner .odev of the bridge
>>   drm/bridge: analogix-anx78xx: provide an owner .odev device
>>   drm/bridge: cdns-dsi: provide an owner .odev device
>>   drm/bridge: vga-dac: provide an owner .odev device
>>   drm/bridge: lvds-encoder: provide an owner .odev device
>>   drm/bridge: megachips-stdp-ge-b850v3-fw: provide an owner .odev
>> device
>>   drm/bridge: nxp-ptn3460: provide an owner .odev device
>>   drm/bridge: panel: provide an owner .odev device
>>   drm/bridge: ps8622: provide an owner .odev device
>>   drm/bridge: sii902x: provide an owner .odev device
>>   drm/bridge: sii9234: provide an owner .odev device
>>   drm/bridge: sii8620: provide an owner .odev device
>>   drm/bridge: synopsys: provide an owner .odev device for the bridges
>>   drm/bridge: tc358767: provide an owner .odev device
>>   drm/bridge: thc63lvd1024: provide an owner .odev device
>>   drm/bridge: ti-tfp410: provide an owner .odev device
>>   drm/exynos: mic: provide an owner .odev device for the bridge
>>   drm/mediatek: hdmi: provide an owner .odev device for the bridge
>>   drm/msm: specify the owner .odev of the bridges
>>   drm/rcar-du: lvds: provide an owner .odev device for the bridge
>>   drm/sti: provide an owner .odev device for the bridges
>>   drm/bridge: remove the .of_node member
>>   drm/bridge: require the owner .odev to be filled in on
>> drm_bridge_add/attach
>>   drm/bridge: establish a link between the bridge supplier and consumer
>>
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
>>  drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
>>  drivers/gpu/drm/bridge/cdns-dsi.c  |  

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:39, Daniel Vetter wrote:
> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>> but there are several other bridges that never call drm_bridge_add.
>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>
>>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>>
>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>
>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>   drm driver to connect/disconnect a drm_bridge.
>>>
>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>   driver itself to register/unregister itself. Maybe we should rename
>>>   them, since the same issue happens with drm_panel, with the same
>>>   confusion.
>>>
>>> I thought someone was working on a cleanup series to fix this mess, but I
>>> didn't find anything.
>>
>> Ok, I just spotted the imbalance and didn't really dig into what
>> actually happens in these error paths. Now that I have done so I
>> believe that the removed drm_bridge_remove calls causes NULL
>> dereferences if/when the error paths are triggered.
>>
>> So, I don't think this can wait for some bigger cleanup.
>>
>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>> __list_del with NULL in both prev and next since the list member
>> is never initialized. prev and next are dereferenced by __list_del
>> and you have *boom*
>>
>> I recommend adding the tag
>>
>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>
>> so that stable picks this one up.
> 
> I just wanted to correct your commit message text - the correct solution
> is definitely _not_ for sti here to call drm_bridge_add.

Ah, I see what you mean. Do you want me to respin?

>  It should call
> drm_bridge_attach/detach only, as a pair.

Alas, the attach/detach functions are generally not called from the same
level. After the bridge has been attached to an encoder, it is detached
in the generic code shutting down the encoder, i.e. the bridge consumer
is not explicitly involved with bridge detaching.

> I didn't check whether you instead have a _detach call missing or what's
> going on here.

So, even though there is no _detach call, it is still not "missing" as
it is not supposed to be there...

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>>> -Daniel
>>>
>>>> ---
>>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>>>  2 files changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>>>> index 67bbdb49fffc..199db13f565c 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hda.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct 
>>>> device *master, void *data)
>>>>return 0;
>>>>  
>>>>  err_sysfs:
>>>> -  drm_bridge_remove(bridge);
>>>>return -EINVAL;
>>>>  }
>>>>  
>>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c 
>>>> b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> index 58f431102512..932724784942 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct 
>>>> device *master, void *data)
>>>>return 0;
>>>>  
>>>>  err_sysfs:
>>>> -  drm_bridge_remove(bridge);
>>>>hdmi->drm_connector = NULL;
>>>>return -EINVAL;
>>>>  }
>>>> -- 
>>>> 2.11.0
>>>>
>>>> ___
>>>> dri-devel mailing list
>>>> dri-de...@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:39, Daniel Vetter wrote:
> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>> but there are several other bridges that never call drm_bridge_add.
>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>
>>>> Signed-off-by: Peter Rosin 
>>>
>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>
>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>   drm driver to connect/disconnect a drm_bridge.
>>>
>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>   driver itself to register/unregister itself. Maybe we should rename
>>>   them, since the same issue happens with drm_panel, with the same
>>>   confusion.
>>>
>>> I thought someone was working on a cleanup series to fix this mess, but I
>>> didn't find anything.
>>
>> Ok, I just spotted the imbalance and didn't really dig into what
>> actually happens in these error paths. Now that I have done so I
>> believe that the removed drm_bridge_remove calls causes NULL
>> dereferences if/when the error paths are triggered.
>>
>> So, I don't think this can wait for some bigger cleanup.
>>
>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>> __list_del with NULL in both prev and next since the list member
>> is never initialized. prev and next are dereferenced by __list_del
>> and you have *boom*
>>
>> I recommend adding the tag
>>
>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>
>> so that stable picks this one up.
> 
> I just wanted to correct your commit message text - the correct solution
> is definitely _not_ for sti here to call drm_bridge_add.

Ah, I see what you mean. Do you want me to respin?

>  It should call
> drm_bridge_attach/detach only, as a pair.

Alas, the attach/detach functions are generally not called from the same
level. After the bridge has been attached to an encoder, it is detached
in the generic code shutting down the encoder, i.e. the bridge consumer
is not explicitly involved with bridge detaching.

> I didn't check whether you instead have a _detach call missing or what's
> going on here.

So, even though there is no _detach call, it is still not "missing" as
it is not supposed to be there...

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>>> -Daniel
>>>
>>>> ---
>>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>>>  2 files changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>>>> index 67bbdb49fffc..199db13f565c 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hda.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct 
>>>> device *master, void *data)
>>>>return 0;
>>>>  
>>>>  err_sysfs:
>>>> -  drm_bridge_remove(bridge);
>>>>return -EINVAL;
>>>>  }
>>>>  
>>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c 
>>>> b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> index 58f431102512..932724784942 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct 
>>>> device *master, void *data)
>>>>return 0;
>>>>  
>>>>  err_sysfs:
>>>> -  drm_bridge_remove(bridge);
>>>>hdmi->drm_connector = NULL;
>>>>return -EINVAL;
>>>>  }
>>>> -- 
>>>> 2.11.0
>>>>
>>>> ___
>>>> dri-devel mailing list
>>>> dri-de...@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-07 14:59, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded.

The step is very small IMHO. Just a device-link, which is very easy to
remove once whatever other solution is ready.

>   Do we need to go this way?

If the drivers expect the parts to be there, and there is no other safety
net in place if they are not, what is the (short-term) alternative?

> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.

This patch does not contribute to any late init and any such delay is not
affected by this. At all.

> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

That is a much bigger question than this patch/series. Conflating the
two is not fair IMHO. You could run this very same argument for every
driver that gets added, since any additional driver will just make it
harder to make everything dynamic. Should we stop development right
away?

Besides, as long as the drm devices are in fact acting as big static
drivers (built from smaller parts), this should be considered a bug-fix
that will prevent dereference of stale pointers.

Or will some other solution appear and magically make all bridges and
drm drivers capable of dynamic reconfiguration in the next few weeks?
Yeah, right...

Cheers,
Peter

> Regards
> Andrzej
> 
> 
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>  include/drm/drm_bridge.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include 
>>  
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>> struct drm_bridge *bridge,
>>  if (bridge->dev)
>>  return -EBUSY;
>>  
>> +if (encoder->dev->dev != bridge->odev) {
>> +bridge->link = device_link_add(encoder->dev->dev,
>> +   bridge->odev, 0);
>> +if (!bridge->link) {
>> +dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +dev_name(encoder->dev->dev));
>> +return -EINVAL;
>> +}
>> +}
>> +
>>  bridge->dev = encoder->dev;
>>  bridge->encoder = encoder;
>>  
>>  if (bridge->funcs->attach) {
>>  ret = bridge->funcs->attach(bridge);
>>  if (ret < 0) {
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>>  bridge->dev = NULL;
>>  bridge->encoder = NULL;
>>  return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  if (bridge->funcs->detach)
>>  bridge->funcs->detach(bridge);
>>  
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>> +
>>  bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  struct drm_bridge *next;
>>  struct list_head list;
>>  const struct drm_bridge_timings *timings;
>> +struct device_link *link;
>>  
>>  const struct drm_bridge_funcs *funcs;
>>  void *driver_private;
> 
> 

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-07 14:59, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded.

The step is very small IMHO. Just a device-link, which is very easy to
remove once whatever other solution is ready.

>   Do we need to go this way?

If the drivers expect the parts to be there, and there is no other safety
net in place if they are not, what is the (short-term) alternative?

> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.

This patch does not contribute to any late init and any such delay is not
affected by this. At all.

> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

That is a much bigger question than this patch/series. Conflating the
two is not fair IMHO. You could run this very same argument for every
driver that gets added, since any additional driver will just make it
harder to make everything dynamic. Should we stop development right
away?

Besides, as long as the drm devices are in fact acting as big static
drivers (built from smaller parts), this should be considered a bug-fix
that will prevent dereference of stale pointers.

Or will some other solution appear and magically make all bridges and
drm drivers capable of dynamic reconfiguration in the next few weeks?
Yeah, right...

Cheers,
Peter

> Regards
> Andrzej
> 
> 
>>
>> Signed-off-by: Peter Rosin 
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++
>>  include/drm/drm_bridge.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include 
>>  
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, 
>> struct drm_bridge *bridge,
>>  if (bridge->dev)
>>  return -EBUSY;
>>  
>> +if (encoder->dev->dev != bridge->odev) {
>> +bridge->link = device_link_add(encoder->dev->dev,
>> +   bridge->odev, 0);
>> +if (!bridge->link) {
>> +dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +dev_name(encoder->dev->dev));
>> +return -EINVAL;
>> +}
>> +}
>> +
>>  bridge->dev = encoder->dev;
>>  bridge->encoder = encoder;
>>  
>>  if (bridge->funcs->attach) {
>>  ret = bridge->funcs->attach(bridge);
>>  if (ret < 0) {
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>>  bridge->dev = NULL;
>>  bridge->encoder = NULL;
>>  return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  if (bridge->funcs->detach)
>>  bridge->funcs->detach(bridge);
>>  
>> +if (bridge->link)
>> +device_link_del(bridge->link);
>> +bridge->link = NULL;
>> +
>>  bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  struct drm_bridge *next;
>>  struct list_head list;
>>  const struct drm_bridge_timings *timings;
>> +struct device_link *link;
>>  
>>  const struct drm_bridge_funcs *funcs;
>>  void *driver_private;
> 
> 

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-05 03:43, Wenwen Wang wrote:
> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
> which are used to save a series of messages, as mentioned in the comment.
> According to the value of the variable "size", msgbuf0 is initialized to
> various values. In contrast, msgbuf1 is left uninitialized until the
> function i2c_transfer() is invoked. However, mgsbuf1 is not always
> initialized on all possible execution paths (implementation) of
> i2c_transfer(). Thus, it is possible that mgsbuf1 may still be
> uninitialized even after the invocation of the function i2c_transfer(),
> especially when the return value of ic2_transfer() is not checked properly.
> In the following execution, the uninitialized msgbuf1 will be used, such as
> for security checks. Since uninitialized values can be random and
> arbitrary, this will cause undefined behaviors or even check bypass. For
> example, it is expected that if the value of "size" is
> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
> value read from msgbuf1 is assigned to data->block[0], which can
> potentially lead to invalid block write size, as demonstrated in the error
> message.
> 
> This patch checks the return value of i2c_transfer() and also initializes
> the first byte of msgbuf1 with 0 to avoid undefined behaviors or security
> issues.
> 
> Signed-off-by: Wenwen Wang 
> ---
>  drivers/i2c/i2c-core-smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b5aec33..e8470d5 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -344,6 +344,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   };
>  
>   msgbuf0[0] = command;
> + msgbug1[0] = 0;
>   switch (size) {
>   case I2C_SMBUS_QUICK:
>   msg[0].len = 0;
> @@ -466,6 +467,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   status = i2c_transfer(adapter, msg, num);
>   if (status < 0)
>   return status;
> + if (status != num)
> + return -EIO;
>  
>   /* Check PEC if last message is a read */
>   if (i && (msg[num-1].flags & I2C_M_RD)) {
> 

I think these two hunks should be two separate patches. They address
orthogonal issues...

Cheers,
Peter

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-05 03:43, Wenwen Wang wrote:
> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
> which are used to save a series of messages, as mentioned in the comment.
> According to the value of the variable "size", msgbuf0 is initialized to
> various values. In contrast, msgbuf1 is left uninitialized until the
> function i2c_transfer() is invoked. However, mgsbuf1 is not always
> initialized on all possible execution paths (implementation) of
> i2c_transfer(). Thus, it is possible that mgsbuf1 may still be
> uninitialized even after the invocation of the function i2c_transfer(),
> especially when the return value of ic2_transfer() is not checked properly.
> In the following execution, the uninitialized msgbuf1 will be used, such as
> for security checks. Since uninitialized values can be random and
> arbitrary, this will cause undefined behaviors or even check bypass. For
> example, it is expected that if the value of "size" is
> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
> value read from msgbuf1 is assigned to data->block[0], which can
> potentially lead to invalid block write size, as demonstrated in the error
> message.
> 
> This patch checks the return value of i2c_transfer() and also initializes
> the first byte of msgbuf1 with 0 to avoid undefined behaviors or security
> issues.
> 
> Signed-off-by: Wenwen Wang 
> ---
>  drivers/i2c/i2c-core-smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b5aec33..e8470d5 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -344,6 +344,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   };
>  
>   msgbuf0[0] = command;
> + msgbug1[0] = 0;
>   switch (size) {
>   case I2C_SMBUS_QUICK:
>   msg[0].len = 0;
> @@ -466,6 +467,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>   status = i2c_transfer(adapter, msg, num);
>   if (status < 0)
>   return status;
> + if (status != num)
> + return -EIO;
>  
>   /* Check PEC if last message is a read */
>   if (i && (msg[num-1].flags & I2C_M_RD)) {
> 

I think these two hunks should be two separate patches. They address
orthogonal issues...

Cheers,
Peter

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 16:59, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 2:27 AM, Peter Rosin <p...@axentia.se> wrote:
>> On 2018-05-04 09:17, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <p...@axentia.se> wrote:
>>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <p...@axentia.se> wrote:
>>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <p...@axentia.se> wrote:
>>>>>>>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>>>>>>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and 
>>>>>>>>> msgbuf1,
>>>>>>>>> which are used to save a series of messages, as mentioned in the 
>>>>>>>>> comment.
>>>>>>>>> According to the value of the variable "size", msgbuf0 is initialized 
>>>>>>>>> to
>>>>>>>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>>>>>>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>>>
>>>>>>>> double negation here
>>>>>>>>
>>>>>>>>> uninitialized even after the invocation of the function 
>>>>>>>>> i2c_transfer(). In
>>>>>>>>> the following execution, the uninitialized msgbuf1 will be used, such 
>>>>>>>>> as
>>>>>>>>> for security checks. Since uninitialized values can be random and
>>>>>>>>> arbitrary, this will cause undefined behaviors or even check bypass. 
>>>>>>>>> For
>>>>>>>>> example, it is expected that if the value of "size" is
>>>>>>>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be 
>>>>>>>>> larger
>>>>>>>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of 
>>>>>>>>> i2c_smbus_xfer_emulated(), the
>>>>>>>>> value read from msgbuf1 is assigned to data->block[0], which can
>>>>>>>>> potentially lead to invalid block write size, as demonstrated in the 
>>>>>>>>> error
>>>>>>>>> message.
>>>>>>>>>
>>>>>>>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid 
>>>>>>>>> undefined
>>>>>>>>> behaviors or security issues.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wenwen Wang <wang6...@umn.edu>
>>>>>>>>> ---
>>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c 
>>>>>>>>> b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct 
>>>>>>>>> i2c_adapter *adapter, u16 addr,
>>>>>>>>>* somewhat simpler.
>>>>>>>>>*/
>>>>>>>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>>
>>>>>>>> I think this will result in the whole of msgbuf1 being filled with 
>>>>>>>> zeroes.
>>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>>> initializer?
>>>>>>>
>>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>&

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 16:59, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 2:27 AM, Peter Rosin  wrote:
>> On 2018-05-04 09:17, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin  wrote:
>>>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin  wrote:
>>>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin  wrote:
>>>>>>>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>>>>>>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and 
>>>>>>>>> msgbuf1,
>>>>>>>>> which are used to save a series of messages, as mentioned in the 
>>>>>>>>> comment.
>>>>>>>>> According to the value of the variable "size", msgbuf0 is initialized 
>>>>>>>>> to
>>>>>>>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>>>>>>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>>>
>>>>>>>> double negation here
>>>>>>>>
>>>>>>>>> uninitialized even after the invocation of the function 
>>>>>>>>> i2c_transfer(). In
>>>>>>>>> the following execution, the uninitialized msgbuf1 will be used, such 
>>>>>>>>> as
>>>>>>>>> for security checks. Since uninitialized values can be random and
>>>>>>>>> arbitrary, this will cause undefined behaviors or even check bypass. 
>>>>>>>>> For
>>>>>>>>> example, it is expected that if the value of "size" is
>>>>>>>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be 
>>>>>>>>> larger
>>>>>>>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of 
>>>>>>>>> i2c_smbus_xfer_emulated(), the
>>>>>>>>> value read from msgbuf1 is assigned to data->block[0], which can
>>>>>>>>> potentially lead to invalid block write size, as demonstrated in the 
>>>>>>>>> error
>>>>>>>>> message.
>>>>>>>>>
>>>>>>>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid 
>>>>>>>>> undefined
>>>>>>>>> behaviors or security issues.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wenwen Wang 
>>>>>>>>> ---
>>>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c 
>>>>>>>>> b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> index b5aec33..0fcca75 100644
>>>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct 
>>>>>>>>> i2c_adapter *adapter, u16 addr,
>>>>>>>>>* somewhat simpler.
>>>>>>>>>*/
>>>>>>>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>>>
>>>>>>>> I think this will result in the whole of msgbuf1 being filled with 
>>>>>>>> zeroes.
>>>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>>>> initializer?
>>>>>>>
>>>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>>>> used only in that case?
>>>>>>
>>>>>> I was thinking that an assignm

[PATCH v2 03/26] drm/bridge/analogix: core: specify the owner .odev of the bridge

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 2bcbfadb6ac5..c60f29216213 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1467,6 +1467,7 @@ static int analogix_dp_create_bridge(struct drm_device 
*drm_dev,
 
dp->bridge = bridge;
 
+   bridge->odev = dp->dev;
bridge->driver_private = dp;
bridge->funcs = _dp_bridge_funcs;
 
-- 
2.11.0

[PATCH v2 03/26] drm/bridge/analogix: core: specify the owner .odev of the bridge

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 2bcbfadb6ac5..c60f29216213 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1467,6 +1467,7 @@ static int analogix_dp_create_bridge(struct drm_device 
*drm_dev,
 
dp->bridge = bridge;
 
+   bridge->odev = dp->dev;
bridge->driver_private = dp;
bridge->funcs = _dp_bridge_funcs;
 
-- 
2.11.0

[PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner device or keep just providing an
of_node.

By providing a full owner device, the bridge drivers no longer need
to provide an of_node since that node is available via the owner
device.

When all bridge drivers provide an owner device, that will become
mandatory and the .of_node member will be removed.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 ++-
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
 include/drm/drm_bridge.h | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3872f5379998 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if (bridge->of_node == np) {
+   if ((bridge->odev && bridge->odev->of_node == np) ||
+   bridge->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..557e0079c98d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else
+   else if (lvds->bridge->of_node)
remote = lvds->bridge->of_node;
+   else
+   remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
/* default set it as output rgb */
lvds->output = DISPLAY_OUTPUT_RGB;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7c17977c3537 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -254,6 +254,7 @@ struct drm_bridge_timings {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @odev: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -265,6 +266,7 @@ struct drm_bridge_timings {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+   struct device *odev;
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-- 
2.11.0

[PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner device or keep just providing an
of_node.

By providing a full owner device, the bridge drivers no longer need
to provide an of_node since that node is available via the owner
device.

When all bridge drivers provide an owner device, that will become
mandatory and the .of_node member will be removed.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/drm_bridge.c | 3 ++-
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
 include/drm/drm_bridge.h | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3872f5379998 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if (bridge->of_node == np) {
+   if ((bridge->odev && bridge->odev->of_node == np) ||
+   bridge->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..557e0079c98d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else
+   else if (lvds->bridge->of_node)
remote = lvds->bridge->of_node;
+   else
+   remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
/* default set it as output rgb */
lvds->output = DISPLAY_OUTPUT_RGB;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7c17977c3537 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -254,6 +254,7 @@ struct drm_bridge_timings {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @odev: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -265,6 +266,7 @@ struct drm_bridge_timings {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+   struct device *odev;
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-- 
2.11.0

[PATCH v2 04/26] drm/bridge: analogix-anx78xx: provide an owner .odev device

[Peter Rosin]

It gets rid of an #if and the .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c 
b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index b49043866be6..1d6620aedbdb 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1332,10 +1332,6 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 
mutex_init(>lock);
 
-#if IS_ENABLED(CONFIG_OF)
-   anx78xx->bridge.of_node = client->dev.of_node;
-#endif
-
anx78xx->client = client;
i2c_set_clientdata(client, anx78xx);
 
@@ -1433,6 +1429,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
goto err_poweroff;
}
 
+   anx78xx->bridge.odev = >dev;
anx78xx->bridge.funcs = _bridge_funcs;
 
drm_bridge_add(>bridge);
-- 
2.11.0

[PATCH v2 04/26] drm/bridge: analogix-anx78xx: provide an owner .odev device

[Peter Rosin]

It gets rid of an #if and the .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c 
b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index b49043866be6..1d6620aedbdb 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1332,10 +1332,6 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 
mutex_init(>lock);
 
-#if IS_ENABLED(CONFIG_OF)
-   anx78xx->bridge.of_node = client->dev.of_node;
-#endif
-
anx78xx->client = client;
i2c_set_clientdata(client, anx78xx);
 
@@ -1433,6 +1429,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
goto err_poweroff;
}
 
+   anx78xx->bridge.odev = >dev;
anx78xx->bridge.funcs = _bridge_funcs;
 
drm_bridge_add(>bridge);
-- 
2.11.0

[PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

Hi!

It was noted by Russel King [1] that bridges (not using components)
might disappear unexpectedly if the owner of the bridge was unbound.
Jyri Sarha had previously noted the same thing with panels [2]. Jyri
came up with using device links to resolve the panel issue, which
was also my (independent) reaction to the note from Russel.

This series builds up to the addition of that link in the last
patch, but in my opinion the other 25 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. Jyri, are
you able to test? That said, I might have missed some subtlety.

Oh and the reason I'm pushing this is of course so that the issue
noted by Russel in [1] is addressed which in turn means that the
tda998x bridge driver can be patched according to that series without
objection (hopefully) and then used from the atmel-hlcdc driver (and
other drivers that are not componentized).

Changes since v1https://lkml.org/lkml/2018/4/26/1018

- rename .owner to .odev to not get mixed up with the module owner.
- added patches for new recent drivers thc63lvd1024 and cdns-dsi
- fix for problem in the rockchip_lvds driver reported by 0day
- added a WARN in drm_bridge_add if there is no .odev owner device

I did *not*:
- add any ack from Daniel since he suggested "pdev", and I ended up
  with "odev" in the rename since I disliked "pdev" about as much
  as "owner".
- add any port id. The current .of_node (that this series removes)
  does not identify the port, so that problem seems orthogonal
  to me.

Cheers,
Peter

[1] https://lkml.org/lkml/2018/4/23/769
[2] https://www.spinics.net/lists/dri-devel/msg174275.html

Peter Rosin (26):
  drm/bridge: allow optionally specifying an owner .odev device
  drm/bridge: adv7511: provide an owner .odev device
  drm/bridge/analogix: core: specify the owner .odev of the bridge
  drm/bridge: analogix-anx78xx: provide an owner .odev device
  drm/bridge: cdns-dsi: provide an owner .odev device
  drm/bridge: vga-dac: provide an owner .odev device
  drm/bridge: lvds-encoder: provide an owner .odev device
  drm/bridge: megachips-stdp-ge-b850v3-fw: provide an owner .odev
device
  drm/bridge: nxp-ptn3460: provide an owner .odev device
  drm/bridge: panel: provide an owner .odev device
  drm/bridge: ps8622: provide an owner .odev device
  drm/bridge: sii902x: provide an owner .odev device
  drm/bridge: sii9234: provide an owner .odev device
  drm/bridge: sii8620: provide an owner .odev device
  drm/bridge: synopsys: provide an owner .odev device for the bridges
  drm/bridge: tc358767: provide an owner .odev device
  drm/bridge: thc63lvd1024: provide an owner .odev device
  drm/bridge: ti-tfp410: provide an owner .odev device
  drm/exynos: mic: provide an owner .odev device for the bridge
  drm/mediatek: hdmi: provide an owner .odev device for the bridge
  drm/msm: specify the owner .odev of the bridges
  drm/rcar-du: lvds: provide an owner .odev device for the bridge
  drm/sti: provide an owner .odev device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the owner .odev to be filled in on
drm_bridge_add/attach
  drm/bridge: establish a link between the bridge supplier and consumer

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
 drivers/gpu/drm/bridge/cdns-dsi.c  |  2 +-
 drivers/gpu/drm/bridge/dumb-vga-dac.c  |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c  |  2 +-
 .../drm/bridge/megachips-stdp-ge-b850v3-fw.c   |  2 +-
 drivers/gpu/drm/bridge/nxp-ptn3460.c   |  2 +-
 drivers/gpu/drm/bridge/panel.c |  4 +---
 drivers/gpu/drm/bridge/parade-ps8622.c |  2 +-
 drivers/gpu/drm/bridge/sii902x.c   |  2 +-
 drivers/gpu/drm/bridge/sii9234.c   |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c   |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c  |  4 +---
 drivers/gpu/drm/bridge/tc358767.c  |  2 +-
 drivers/gpu/drm/bridge/thc63lvd1024.c  |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c |  2 +-
 drivers/gpu/drm/drm_bridge.c   | 26 +-
 drivers/gpu/drm/exynos/exynos_drm_mic.c|  2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c|  2 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c  |  1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c|  2 +-
 drivers/gpu/drm/rockchip/rockchip_lvds.c   |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c  |  2 +-
 drivers/gpu/drm/sti/sti_hda.c  |  1 +
 drivers/gpu/drm

[PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

Hi!

It was noted by Russel King [1] that bridges (not using components)
might disappear unexpectedly if the owner of the bridge was unbound.
Jyri Sarha had previously noted the same thing with panels [2]. Jyri
came up with using device links to resolve the panel issue, which
was also my (independent) reaction to the note from Russel.

This series builds up to the addition of that link in the last
patch, but in my opinion the other 25 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. Jyri, are
you able to test? That said, I might have missed some subtlety.

Oh and the reason I'm pushing this is of course so that the issue
noted by Russel in [1] is addressed which in turn means that the
tda998x bridge driver can be patched according to that series without
objection (hopefully) and then used from the atmel-hlcdc driver (and
other drivers that are not componentized).

Changes since v1https://lkml.org/lkml/2018/4/26/1018

- rename .owner to .odev to not get mixed up with the module owner.
- added patches for new recent drivers thc63lvd1024 and cdns-dsi
- fix for problem in the rockchip_lvds driver reported by 0day
- added a WARN in drm_bridge_add if there is no .odev owner device

I did *not*:
- add any ack from Daniel since he suggested "pdev", and I ended up
  with "odev" in the rename since I disliked "pdev" about as much
  as "owner".
- add any port id. The current .of_node (that this series removes)
  does not identify the port, so that problem seems orthogonal
  to me.

Cheers,
Peter

[1] https://lkml.org/lkml/2018/4/23/769
[2] https://www.spinics.net/lists/dri-devel/msg174275.html

Peter Rosin (26):
  drm/bridge: allow optionally specifying an owner .odev device
  drm/bridge: adv7511: provide an owner .odev device
  drm/bridge/analogix: core: specify the owner .odev of the bridge
  drm/bridge: analogix-anx78xx: provide an owner .odev device
  drm/bridge: cdns-dsi: provide an owner .odev device
  drm/bridge: vga-dac: provide an owner .odev device
  drm/bridge: lvds-encoder: provide an owner .odev device
  drm/bridge: megachips-stdp-ge-b850v3-fw: provide an owner .odev
device
  drm/bridge: nxp-ptn3460: provide an owner .odev device
  drm/bridge: panel: provide an owner .odev device
  drm/bridge: ps8622: provide an owner .odev device
  drm/bridge: sii902x: provide an owner .odev device
  drm/bridge: sii9234: provide an owner .odev device
  drm/bridge: sii8620: provide an owner .odev device
  drm/bridge: synopsys: provide an owner .odev device for the bridges
  drm/bridge: tc358767: provide an owner .odev device
  drm/bridge: thc63lvd1024: provide an owner .odev device
  drm/bridge: ti-tfp410: provide an owner .odev device
  drm/exynos: mic: provide an owner .odev device for the bridge
  drm/mediatek: hdmi: provide an owner .odev device for the bridge
  drm/msm: specify the owner .odev of the bridges
  drm/rcar-du: lvds: provide an owner .odev device for the bridge
  drm/sti: provide an owner .odev device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the owner .odev to be filled in on
drm_bridge_add/attach
  drm/bridge: establish a link between the bridge supplier and consumer

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c  |  5 +
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
 drivers/gpu/drm/bridge/cdns-dsi.c  |  2 +-
 drivers/gpu/drm/bridge/dumb-vga-dac.c  |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c  |  2 +-
 .../drm/bridge/megachips-stdp-ge-b850v3-fw.c   |  2 +-
 drivers/gpu/drm/bridge/nxp-ptn3460.c   |  2 +-
 drivers/gpu/drm/bridge/panel.c |  4 +---
 drivers/gpu/drm/bridge/parade-ps8622.c |  2 +-
 drivers/gpu/drm/bridge/sii902x.c   |  2 +-
 drivers/gpu/drm/bridge/sii9234.c   |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c   |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c  |  4 +---
 drivers/gpu/drm/bridge/tc358767.c  |  2 +-
 drivers/gpu/drm/bridge/thc63lvd1024.c  |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c |  2 +-
 drivers/gpu/drm/drm_bridge.c   | 26 +-
 drivers/gpu/drm/exynos/exynos_drm_mic.c|  2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c|  2 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c  |  1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   |  1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c|  2 +-
 drivers/gpu/drm/rockchip/rockchip_lvds.c   |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c  |  2 +-
 drivers/gpu/drm/sti/sti_hda.c  |  1 +
 drivers/gpu/drm

[PATCH v2 02/26] drm/bridge: adv7511: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2614cea538e2..fd2eef916b0b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1204,8 +1204,8 @@ static int adv7511_probe(struct i2c_client *i2c, const 
struct i2c_device_id *id)
if (ret)
goto err_unregister_cec;
 
+   adv7511->bridge.odev = dev;
adv7511->bridge.funcs = _bridge_funcs;
-   adv7511->bridge.of_node = dev->of_node;
 
drm_bridge_add(>bridge);
 
-- 
2.11.0

[PATCH v2 05/26] drm/bridge: cdns-dsi: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/cdns-dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c 
b/drivers/gpu/drm/bridge/cdns-dsi.c
index c255fc3e1be5..e9be5c3f0284 100644
--- a/drivers/gpu/drm/bridge/cdns-dsi.c
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -1548,8 +1548,8 @@ static int cdns_dsi_drm_probe(struct platform_device 
*pdev)
 * CDNS_DPI_INPUT.
 */
input->id = CDNS_DPI_INPUT;
+   input->bridge.odev = >dev;
input->bridge.funcs = _dsi_bridge_funcs;
-   input->bridge.of_node = pdev->dev.of_node;
 
/* Mask all interrupts before registering the IRQ handler. */
writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
-- 
2.11.0

[PATCH v2 02/26] drm/bridge: adv7511: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2614cea538e2..fd2eef916b0b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1204,8 +1204,8 @@ static int adv7511_probe(struct i2c_client *i2c, const 
struct i2c_device_id *id)
if (ret)
goto err_unregister_cec;
 
+   adv7511->bridge.odev = dev;
adv7511->bridge.funcs = _bridge_funcs;
-   adv7511->bridge.of_node = dev->of_node;
 
drm_bridge_add(>bridge);
 
-- 
2.11.0

[PATCH v2 05/26] drm/bridge: cdns-dsi: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/cdns-dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c 
b/drivers/gpu/drm/bridge/cdns-dsi.c
index c255fc3e1be5..e9be5c3f0284 100644
--- a/drivers/gpu/drm/bridge/cdns-dsi.c
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -1548,8 +1548,8 @@ static int cdns_dsi_drm_probe(struct platform_device 
*pdev)
 * CDNS_DPI_INPUT.
 */
input->id = CDNS_DPI_INPUT;
+   input->bridge.odev = >dev;
input->bridge.funcs = _dsi_bridge_funcs;
-   input->bridge.of_node = pdev->dev.of_node;
 
/* Mask all interrupts before registering the IRQ handler. */
writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
-- 
2.11.0

[PATCH v2 08/26] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c 
b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
index 7ccadba7c98c..29d1b5ae5fb6 100644
--- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
@@ -313,8 +313,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client 
*stdp4028_i2c,
i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
 
/* drm bridge initialization */
+   ge_b850v3_lvds_ptr->bridge.odev = dev;
ge_b850v3_lvds_ptr->bridge.funcs = _b850v3_lvds_funcs;
-   ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
drm_bridge_add(_b850v3_lvds_ptr->bridge);
 
/* Clear pending interrupts since power up. */
-- 
2.11.0

[PATCH v2 08/26] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c 
b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
index 7ccadba7c98c..29d1b5ae5fb6 100644
--- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
@@ -313,8 +313,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client 
*stdp4028_i2c,
i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
 
/* drm bridge initialization */
+   ge_b850v3_lvds_ptr->bridge.odev = dev;
ge_b850v3_lvds_ptr->bridge.funcs = _b850v3_lvds_funcs;
-   ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
drm_bridge_add(_b850v3_lvds_ptr->bridge);
 
/* Clear pending interrupts since power up. */
-- 
2.11.0

[PATCH v2 06/26] drm/bridge: vga-dac: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c 
b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 9837c8d69e69..95cce18e8943 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -205,8 +205,8 @@ static int dumb_vga_probe(struct platform_device *pdev)
}
}
 
+   vga->bridge.odev = >dev;
vga->bridge.funcs = _vga_bridge_funcs;
-   vga->bridge.of_node = pdev->dev.of_node;
vga->bridge.timings = of_device_get_match_data(>dev);
 
drm_bridge_add(>bridge);
-- 
2.11.0

[PATCH v2 06/26] drm/bridge: vga-dac: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c 
b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 9837c8d69e69..95cce18e8943 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -205,8 +205,8 @@ static int dumb_vga_probe(struct platform_device *pdev)
}
}
 
+   vga->bridge.odev = >dev;
vga->bridge.funcs = _vga_bridge_funcs;
-   vga->bridge.of_node = pdev->dev.of_node;
vga->bridge.timings = of_device_get_match_data(>dev);
 
drm_bridge_add(>bridge);
-- 
2.11.0

[PATCH v2 07/26] drm/bridge: lvds-encoder: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/lvds-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c 
b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..a80eec17b13b 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -83,7 +83,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 * but we need a bridge attached to our of_node for our user
 * to look up.
 */
-   lvds_encoder->bridge.of_node = pdev->dev.of_node;
+   lvds_encoder->bridge.odev = >dev;
lvds_encoder->bridge.funcs = 
drm_bridge_add(_encoder->bridge);
 
-- 
2.11.0

[PATCH v2 07/26] drm/bridge: lvds-encoder: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/lvds-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c 
b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..a80eec17b13b 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -83,7 +83,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 * but we need a bridge attached to our of_node for our user
 * to look up.
 */
-   lvds_encoder->bridge.of_node = pdev->dev.of_node;
+   lvds_encoder->bridge.odev = >dev;
lvds_encoder->bridge.funcs = 
drm_bridge_add(_encoder->bridge);
 
-- 
2.11.0

[PATCH v2 12/26] drm/bridge: sii902x: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/sii902x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..894525b05985 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -427,8 +427,8 @@ static int sii902x_probe(struct i2c_client *client,
return ret;
}
 
+   sii902x->bridge.odev = dev;
sii902x->bridge.funcs = _bridge_funcs;
-   sii902x->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
i2c_set_clientdata(client, sii902x);
-- 
2.11.0

[PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Peter Rosin]

It gets rid of an #ifdef and the .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/panel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 6d99d4a3beb3..f43d77b5ed20 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel 
*panel,
panel_bridge->connector_type = connector_type;
panel_bridge->panel = panel;
 
+   panel_bridge->bridge.odev = panel->dev;
panel_bridge->bridge.funcs = _bridge_bridge_funcs;
-#ifdef CONFIG_OF
-   panel_bridge->bridge.of_node = panel->dev->of_node;
-#endif
 
drm_bridge_add(_bridge->bridge);
 
-- 
2.11.0

[PATCH v2 12/26] drm/bridge: sii902x: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/sii902x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..894525b05985 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -427,8 +427,8 @@ static int sii902x_probe(struct i2c_client *client,
return ret;
}
 
+   sii902x->bridge.odev = dev;
sii902x->bridge.funcs = _bridge_funcs;
-   sii902x->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
i2c_set_clientdata(client, sii902x);
-- 
2.11.0

[PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Peter Rosin]

It gets rid of an #ifdef and the .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/panel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 6d99d4a3beb3..f43d77b5ed20 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel 
*panel,
panel_bridge->connector_type = connector_type;
panel_bridge->panel = panel;
 
+   panel_bridge->bridge.odev = panel->dev;
panel_bridge->bridge.funcs = _bridge_bridge_funcs;
-#ifdef CONFIG_OF
-   panel_bridge->bridge.of_node = panel->dev->of_node;
-#endif
 
drm_bridge_add(_bridge->bridge);
 
-- 
2.11.0

[PATCH v2 13/26] drm/bridge: sii9234: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/sii9234.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index c77000626c22..54326357b2ee 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -948,8 +948,8 @@ static int sii9234_probe(struct i2c_client *client,
 
i2c_set_clientdata(client, ctx);
 
+   ctx->bridge.odev = dev;
ctx->bridge.funcs = _bridge_funcs;
-   ctx->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
sii9234_cable_in(ctx);
-- 
2.11.0

[PATCH v2 13/26] drm/bridge: sii9234: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/sii9234.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index c77000626c22..54326357b2ee 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -948,8 +948,8 @@ static int sii9234_probe(struct i2c_client *client,
 
i2c_set_clientdata(client, ctx);
 
+   ctx->bridge.odev = dev;
ctx->bridge.funcs = _bridge_funcs;
-   ctx->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
sii9234_cable_in(ctx);
-- 
2.11.0

[PATCH v2 15/26] drm/bridge: synopsys: provide an owner .odev device for the bridges

[Peter Rosin]

It gets rid of two #ifdefs and the .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ec8d0006ef7c..e3956a7e827c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2471,11 +2471,9 @@ __dw_hdmi_probe(struct platform_device *pdev,
hdmi->ddc = NULL;
}
 
+   hdmi->bridge.odev = dev;
hdmi->bridge.driver_private = hdmi;
hdmi->bridge.funcs = _hdmi_bridge_funcs;
-#ifdef CONFIG_OF
-   hdmi->bridge.of_node = pdev->dev.of_node;
-#endif
 
dw_hdmi_setup_i2c(hdmi);
if (hdmi->phy.ops->setup_hpd)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index fd7999642cf8..7c8d05f7cecd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -930,11 +930,9 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
return ERR_PTR(ret);
}
 
+   dsi->bridge.odev = dev;
dsi->bridge.driver_private = dsi;
dsi->bridge.funcs = _mipi_dsi_bridge_funcs;
-#ifdef CONFIG_OF
-   dsi->bridge.of_node = pdev->dev.of_node;
-#endif
 
return dsi;
 }
-- 
2.11.0

[PATCH v2 15/26] drm/bridge: synopsys: provide an owner .odev device for the bridges

[Peter Rosin]

It gets rid of two #ifdefs and the .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ec8d0006ef7c..e3956a7e827c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2471,11 +2471,9 @@ __dw_hdmi_probe(struct platform_device *pdev,
hdmi->ddc = NULL;
}
 
+   hdmi->bridge.odev = dev;
hdmi->bridge.driver_private = hdmi;
hdmi->bridge.funcs = _hdmi_bridge_funcs;
-#ifdef CONFIG_OF
-   hdmi->bridge.of_node = pdev->dev.of_node;
-#endif
 
dw_hdmi_setup_i2c(hdmi);
if (hdmi->phy.ops->setup_hpd)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index fd7999642cf8..7c8d05f7cecd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -930,11 +930,9 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
return ERR_PTR(ret);
}
 
+   dsi->bridge.odev = dev;
dsi->bridge.driver_private = dsi;
dsi->bridge.funcs = _mipi_dsi_bridge_funcs;
-#ifdef CONFIG_OF
-   dsi->bridge.of_node = pdev->dev.of_node;
-#endif
 
return dsi;
 }
-- 
2.11.0

[PATCH v2 14/26] drm/bridge: sii8620: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c 
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 7ab36042a822..8e35578b0488 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2387,8 +2387,8 @@ static int sii8620_probe(struct i2c_client *client,
 
i2c_set_clientdata(client, ctx);
 
+   ctx->bridge.odev = dev;
ctx->bridge.funcs = _bridge_funcs;
-   ctx->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
if (!ctx->extcon)
-- 
2.11.0

[PATCH v2 14/26] drm/bridge: sii8620: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c 
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 7ab36042a822..8e35578b0488 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2387,8 +2387,8 @@ static int sii8620_probe(struct i2c_client *client,
 
i2c_set_clientdata(client, ctx);
 
+   ctx->bridge.odev = dev;
ctx->bridge.funcs = _bridge_funcs;
-   ctx->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
if (!ctx->extcon)
-- 
2.11.0

[PATCH v2 17/26] drm/bridge: thc63lvd1024: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/thc63lvd1024.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
b/drivers/gpu/drm/bridge/thc63lvd1024.c
index c8b9edd5a7f4..4765c9c45aef 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -167,8 +167,8 @@ static int thc63_probe(struct platform_device *pdev)
if (ret)
return ret;
 
+   thc63->bridge.odev = >dev;
thc63->bridge.driver_private = thc63;
-   thc63->bridge.of_node = pdev->dev.of_node;
thc63->bridge.funcs = _bridge_func;
 
drm_bridge_add(>bridge);
-- 
2.11.0

[PATCH v2 17/26] drm/bridge: thc63lvd1024: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/thc63lvd1024.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
b/drivers/gpu/drm/bridge/thc63lvd1024.c
index c8b9edd5a7f4..4765c9c45aef 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -167,8 +167,8 @@ static int thc63_probe(struct platform_device *pdev)
if (ret)
return ret;
 
+   thc63->bridge.odev = >dev;
thc63->bridge.driver_private = thc63;
-   thc63->bridge.of_node = pdev->dev.of_node;
thc63->bridge.funcs = _bridge_func;
 
drm_bridge_add(>bridge);
-- 
2.11.0

[PATCH v2 19/26] drm/exynos: mic: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/exynos/exynos_drm_mic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c 
b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index 2174814273e2..f9ff8d3ec937 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -417,8 +417,8 @@ static int exynos_mic_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, mic);
 
+   mic->bridge.odev = dev;
mic->bridge.funcs = _bridge_funcs;
-   mic->bridge.of_node = dev->of_node;
 
drm_bridge_add(>bridge);
 
-- 
2.11.0

[PATCH v2 19/26] drm/exynos: mic: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/exynos/exynos_drm_mic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c 
b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index 2174814273e2..f9ff8d3ec937 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -417,8 +417,8 @@ static int exynos_mic_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, mic);
 
+   mic->bridge.odev = dev;
mic->bridge.funcs = _bridge_funcs;
-   mic->bridge.of_node = dev->of_node;
 
drm_bridge_add(>bridge);
 
-- 
2.11.0

[PATCH v2 22/26] drm/rcar-du: lvds: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c 
b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..efda02f55c95 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -463,9 +463,9 @@ static int rcar_lvds_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
 
+   lvds->bridge.odev = >dev;
lvds->bridge.driver_private = lvds;
lvds->bridge.funcs = _lvds_bridge_ops;
-   lvds->bridge.of_node = pdev->dev.of_node;
 
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
lvds->mmio = devm_ioremap_resource(>dev, mem);
-- 
2.11.0

[PATCH v2 20/26] drm/mediatek: hdmi: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c 
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 59a11026dceb..d8c7d93d0a87 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1694,8 +1694,8 @@ static int mtk_drm_hdmi_probe(struct platform_device 
*pdev)
 
mtk_hdmi_register_audio_driver(dev);
 
+   hdmi->bridge.odev = >dev;
hdmi->bridge.funcs = _hdmi_bridge_funcs;
-   hdmi->bridge.of_node = pdev->dev.of_node;
drm_bridge_add(>bridge);
 
ret = mtk_hdmi_clk_enable_audio(hdmi);
-- 
2.11.0

[PATCH v2 22/26] drm/rcar-du: lvds: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c 
b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..efda02f55c95 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -463,9 +463,9 @@ static int rcar_lvds_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
 
+   lvds->bridge.odev = >dev;
lvds->bridge.driver_private = lvds;
lvds->bridge.funcs = _lvds_bridge_ops;
-   lvds->bridge.of_node = pdev->dev.of_node;
 
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
lvds->mmio = devm_ioremap_resource(>dev, mem);
-- 
2.11.0

[PATCH v2 20/26] drm/mediatek: hdmi: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c 
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 59a11026dceb..d8c7d93d0a87 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1694,8 +1694,8 @@ static int mtk_drm_hdmi_probe(struct platform_device 
*pdev)
 
mtk_hdmi_register_audio_driver(dev);
 
+   hdmi->bridge.odev = >dev;
hdmi->bridge.funcs = _hdmi_bridge_funcs;
-   hdmi->bridge.of_node = pdev->dev.of_node;
drm_bridge_add(>bridge);
 
ret = mtk_hdmi_clk_enable_audio(hdmi);
-- 
2.11.0

[PATCH v2 23/26] drm/sti: provide an owner .odev device for the bridges

[Peter Rosin]

The .of_node member is going away and providing an .odev will become
mandatory.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/sti/sti_dvo.c  | 2 +-
 drivers/gpu/drm/sti/sti_hda.c  | 1 +
 drivers/gpu/drm/sti/sti_hdmi.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index a5979cd25cc7..f2609725f8f1 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -460,9 +460,9 @@ static int sti_dvo_bind(struct device *dev, struct device 
*master, void *data)
if (!bridge)
return -ENOMEM;
 
+   bridge->odev = >dev;
bridge->driver_private = dvo;
bridge->funcs = _dvo_bridge_funcs;
-   bridge->of_node = dvo->dev.of_node;
drm_bridge_add(bridge);
 
err = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 199db13f565c..7fcd24664cd0 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -694,6 +694,7 @@ static int sti_hda_bind(struct device *dev, struct device 
*master, void *data)
if (!bridge)
return -ENOMEM;
 
+   bridge->odev = dev;
bridge->driver_private = hda;
bridge->funcs = _hda_bridge_funcs;
drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 932724784942..d1d2e1a1920c 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1270,6 +1270,7 @@ static int sti_hdmi_bind(struct device *dev, struct 
device *master, void *data)
if (!bridge)
return -EINVAL;
 
+   bridge->odev = dev;
bridge->driver_private = hdmi;
bridge->funcs = _hdmi_bridge_funcs;
drm_bridge_attach(encoder, bridge, NULL);
-- 
2.11.0

[PATCH v2 23/26] drm/sti: provide an owner .odev device for the bridges

[Peter Rosin]

The .of_node member is going away and providing an .odev will become
mandatory.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/sti/sti_dvo.c  | 2 +-
 drivers/gpu/drm/sti/sti_hda.c  | 1 +
 drivers/gpu/drm/sti/sti_hdmi.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index a5979cd25cc7..f2609725f8f1 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -460,9 +460,9 @@ static int sti_dvo_bind(struct device *dev, struct device 
*master, void *data)
if (!bridge)
return -ENOMEM;
 
+   bridge->odev = >dev;
bridge->driver_private = dvo;
bridge->funcs = _dvo_bridge_funcs;
-   bridge->of_node = dvo->dev.of_node;
drm_bridge_add(bridge);
 
err = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 199db13f565c..7fcd24664cd0 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -694,6 +694,7 @@ static int sti_hda_bind(struct device *dev, struct device 
*master, void *data)
if (!bridge)
return -ENOMEM;
 
+   bridge->odev = dev;
bridge->driver_private = hda;
bridge->funcs = _hda_bridge_funcs;
drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 932724784942..d1d2e1a1920c 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1270,6 +1270,7 @@ static int sti_hdmi_bind(struct device *dev, struct 
device *master, void *data)
if (!bridge)
return -EINVAL;
 
+   bridge->odev = dev;
bridge->driver_private = hdmi;
bridge->funcs = _hdmi_bridge_funcs;
drm_bridge_attach(encoder, bridge, NULL);
-- 
2.11.0

[PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach

[Peter Rosin]

The .odev owner device will be handy to have around.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index df084db33494..78d186b6831b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+   if (WARN_ON(!bridge->odev))
+   return;
+
mutex_lock(_lock);
list_add_tail(>list, _list);
mutex_unlock(_lock);
@@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (!encoder || !bridge)
return -EINVAL;
 
+   if (WARN_ON(!bridge->odev))
+   return -EINVAL;
+
if (previous && (!previous->dev || previous->encoder != encoder))
return -EINVAL;
 
-- 
2.11.0

[PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach

[Peter Rosin]

The .odev owner device will be handy to have around.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/drm_bridge.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index df084db33494..78d186b6831b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+   if (WARN_ON(!bridge->odev))
+   return;
+
mutex_lock(_lock);
list_add_tail(>list, _list);
mutex_unlock(_lock);
@@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (!encoder || !bridge)
return -EINVAL;
 
+   if (WARN_ON(!bridge->odev))
+   return -EINVAL;
+
if (previous && (!previous->dev || previous->encoder != encoder))
return -EINVAL;
 
-- 
2.11.0

[PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 18 ++
 include/drm/drm_bridge.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 78d186b6831b..0259f0a3ff27 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include "drm_crtc_internal.h"
@@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (bridge->dev)
return -EBUSY;
 
+   if (encoder->dev->dev != bridge->odev) {
+   bridge->link = device_link_add(encoder->dev->dev,
+  bridge->odev, 0);
+   if (!bridge->link) {
+   dev_err(bridge->odev, "failed to link bridge to %s\n",
+   dev_name(encoder->dev->dev));
+   return -EINVAL;
+   }
+   }
+
bridge->dev = encoder->dev;
bridge->encoder = encoder;
 
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
@@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
 
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
+
bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b656e505d11e..804189c63a4c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@ struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: drm consumer <-> bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@ struct drm_bridge {
struct drm_bridge *next;
struct list_head list;
const struct drm_bridge_timings *timings;
+   struct device_link *link;
 
const struct drm_bridge_funcs *funcs;
void *driver_private;
-- 
2.11.0

[PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/drm_bridge.c | 18 ++
 include/drm/drm_bridge.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 78d186b6831b..0259f0a3ff27 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include "drm_crtc_internal.h"
@@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct 
drm_bridge *bridge,
if (bridge->dev)
return -EBUSY;
 
+   if (encoder->dev->dev != bridge->odev) {
+   bridge->link = device_link_add(encoder->dev->dev,
+  bridge->odev, 0);
+   if (!bridge->link) {
+   dev_err(bridge->odev, "failed to link bridge to %s\n",
+   dev_name(encoder->dev->dev));
+   return -EINVAL;
+   }
+   }
+
bridge->dev = encoder->dev;
bridge->encoder = encoder;
 
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
@@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
 
+   if (bridge->link)
+   device_link_del(bridge->link);
+   bridge->link = NULL;
+
bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b656e505d11e..804189c63a4c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@ struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: drm consumer <-> bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@ struct drm_bridge {
struct drm_bridge *next;
struct list_head list;
const struct drm_bridge_timings *timings;
+   struct device_link *link;
 
const struct drm_bridge_funcs *funcs;
void *driver_private;
-- 
2.11.0

[PATCH v2 24/26] drm/bridge: remove the .of_node member

[Peter Rosin]

It is unused.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 +--
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 include/drm/drm_bridge.h | 4 
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 3872f5379998..df084db33494 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,8 +365,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if ((bridge->odev && bridge->odev->of_node == np) ||
-   bridge->of_node == np) {
+   if (bridge->odev->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 557e0079c98d..e77d4c909582 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else if (lvds->bridge->of_node)
-   remote = lvds->bridge->of_node;
else
remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7c17977c3537..b656e505d11e 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,7 +258,6 @@ struct drm_bridge_timings {
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
- * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -270,9 +269,6 @@ struct drm_bridge {
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-#ifdef CONFIG_OF
-   struct device_node *of_node;
-#endif
struct list_head list;
const struct drm_bridge_timings *timings;
 
-- 
2.11.0

[PATCH v2 24/26] drm/bridge: remove the .of_node member

[Peter Rosin]

It is unused.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/drm_bridge.c | 3 +--
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 include/drm/drm_bridge.h | 4 
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 3872f5379998..df084db33494 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,8 +365,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node 
*np)
mutex_lock(_lock);
 
list_for_each_entry(bridge, _list, list) {
-   if ((bridge->odev && bridge->odev->of_node == np) ||
-   bridge->of_node == np) {
+   if (bridge->odev->of_node == np) {
mutex_unlock(_lock);
return bridge;
}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 557e0079c98d..e77d4c909582 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
}
if (lvds->panel)
remote = lvds->panel->dev->of_node;
-   else if (lvds->bridge->of_node)
-   remote = lvds->bridge->of_node;
else
remote = lvds->bridge->odev->of_node;
if (of_property_read_string(dev->of_node, "rockchip,output", ))
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7c17977c3537..b656e505d11e 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,7 +258,6 @@ struct drm_bridge_timings {
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
- * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -270,9 +269,6 @@ struct drm_bridge {
struct drm_device *dev;
struct drm_encoder *encoder;
struct drm_bridge *next;
-#ifdef CONFIG_OF
-   struct device_node *of_node;
-#endif
struct list_head list;
const struct drm_bridge_timings *timings;
 
-- 
2.11.0

[PATCH v2 18/26] drm/bridge: ti-tfp410: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c 
b/drivers/gpu/drm/bridge/ti-tfp410.c
index acb857030951..4745838fdf0e 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -215,8 +215,8 @@ static int tfp410_init(struct device *dev)
return -ENOMEM;
dev_set_drvdata(dev, dvi);
 
+   dvi->bridge.odev = dev;
dvi->bridge.funcs = _bridge_funcs;
-   dvi->bridge.of_node = dev->of_node;
dvi->dev = dev;
 
ret = tfp410_get_connector_properties(dvi);
-- 
2.11.0

[PATCH v2 21/26] drm/msm: specify the owner .odev of the bridges

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c  | 1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   | 1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 4cb1cb68878b..1668e8abe5c1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -710,6 +710,7 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
encoder = msm_dsi->encoder;
 
bridge = _bridge->base;
+   bridge->odev = msm_dsi->dev->dev;
bridge->funcs = _mgr_bridge_funcs;
 
ret = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c 
b/drivers/gpu/drm/msm/edp/edp_bridge.c
index 931a5c97cccf..4c56e29c57b7 100644
--- a/drivers/gpu/drm/msm/edp/edp_bridge.c
+++ b/drivers/gpu/drm/msm/edp/edp_bridge.c
@@ -104,6 +104,7 @@ struct drm_bridge *msm_edp_bridge_init(struct msm_edp *edp)
edp_bridge->edp = edp;
 
bridge = _bridge->base;
+   bridge->odev = edp->dev->dev;
bridge->funcs = _bridge_funcs;
 
ret = drm_bridge_attach(edp->encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c 
b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 7e357077ed26..aa6dd1bc5dc0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -293,6 +293,7 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
hdmi_bridge->hdmi = hdmi;
 
bridge = _bridge->base;
+   bridge->odev = hdmi->dev->dev;
bridge->funcs = _hdmi_bridge_funcs;
 
ret = drm_bridge_attach(hdmi->encoder, bridge, NULL);
-- 
2.11.0

[PATCH v2 16/26] drm/bridge: tc358767: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 0fd9cf27542c..75f93e1d0bf5 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1331,8 +1331,8 @@ static int tc_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
 
tc_connector_set_polling(tc, >connector);
 
+   tc->bridge.odev = dev;
tc->bridge.funcs = _bridge_funcs;
-   tc->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
i2c_set_clientdata(client, tc);
-- 
2.11.0

[PATCH v2 18/26] drm/bridge: ti-tfp410: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c 
b/drivers/gpu/drm/bridge/ti-tfp410.c
index acb857030951..4745838fdf0e 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -215,8 +215,8 @@ static int tfp410_init(struct device *dev)
return -ENOMEM;
dev_set_drvdata(dev, dvi);
 
+   dvi->bridge.odev = dev;
dvi->bridge.funcs = _bridge_funcs;
-   dvi->bridge.of_node = dev->of_node;
dvi->dev = dev;
 
ret = tfp410_get_connector_properties(dvi);
-- 
2.11.0

[PATCH v2 21/26] drm/msm: specify the owner .odev of the bridges

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c  | 1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   | 1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 4cb1cb68878b..1668e8abe5c1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -710,6 +710,7 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
encoder = msm_dsi->encoder;
 
bridge = _bridge->base;
+   bridge->odev = msm_dsi->dev->dev;
bridge->funcs = _mgr_bridge_funcs;
 
ret = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c 
b/drivers/gpu/drm/msm/edp/edp_bridge.c
index 931a5c97cccf..4c56e29c57b7 100644
--- a/drivers/gpu/drm/msm/edp/edp_bridge.c
+++ b/drivers/gpu/drm/msm/edp/edp_bridge.c
@@ -104,6 +104,7 @@ struct drm_bridge *msm_edp_bridge_init(struct msm_edp *edp)
edp_bridge->edp = edp;
 
bridge = _bridge->base;
+   bridge->odev = edp->dev->dev;
bridge->funcs = _bridge_funcs;
 
ret = drm_bridge_attach(edp->encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c 
b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 7e357077ed26..aa6dd1bc5dc0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -293,6 +293,7 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
hdmi_bridge->hdmi = hdmi;
 
bridge = _bridge->base;
+   bridge->odev = hdmi->dev->dev;
bridge->funcs = _hdmi_bridge_funcs;
 
ret = drm_bridge_attach(hdmi->encoder, bridge, NULL);
-- 
2.11.0

[PATCH v2 16/26] drm/bridge: tc358767: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 0fd9cf27542c..75f93e1d0bf5 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1331,8 +1331,8 @@ static int tc_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
 
tc_connector_set_polling(tc, >connector);
 
+   tc->bridge.odev = dev;
tc->bridge.funcs = _bridge_funcs;
-   tc->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
i2c_set_clientdata(client, tc);
-- 
2.11.0

[PATCH v2 11/26] drm/bridge: ps8622: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/parade-ps8622.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c 
b/drivers/gpu/drm/bridge/parade-ps8622.c
index 81198f5e9afa..957420a1c924 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -595,8 +595,8 @@ static int ps8622_probe(struct i2c_client *client,
ps8622->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
}
 
+   ps8622->bridge.odev = dev;
ps8622->bridge.funcs = _bridge_funcs;
-   ps8622->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
i2c_set_clientdata(client, ps8622);
-- 
2.11.0

[PATCH v2 11/26] drm/bridge: ps8622: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/parade-ps8622.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c 
b/drivers/gpu/drm/bridge/parade-ps8622.c
index 81198f5e9afa..957420a1c924 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -595,8 +595,8 @@ static int ps8622_probe(struct i2c_client *client,
ps8622->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
}
 
+   ps8622->bridge.odev = dev;
ps8622->bridge.funcs = _bridge_funcs;
-   ps8622->bridge.of_node = dev->of_node;
drm_bridge_add(>bridge);
 
i2c_set_clientdata(client, ps8622);
-- 
2.11.0

[PATCH v2 09/26] drm/bridge: nxp-ptn3460: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/bridge/nxp-ptn3460.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c 
b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index d64a3283822a..fa832f32b518 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -329,8 +329,8 @@ static int ptn3460_probe(struct i2c_client *client,
return ret;
}
 
+   ptn_bridge->bridge.odev = dev;
ptn_bridge->bridge.funcs = _bridge_funcs;
-   ptn_bridge->bridge.of_node = dev->of_node;
drm_bridge_add(_bridge->bridge);
 
i2c_set_clientdata(client, ptn_bridge);
-- 
2.11.0

[PATCH v2 09/26] drm/bridge: nxp-ptn3460: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/bridge/nxp-ptn3460.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c 
b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index d64a3283822a..fa832f32b518 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -329,8 +329,8 @@ static int ptn3460_probe(struct i2c_client *client,
return ret;
}
 
+   ptn_bridge->bridge.odev = dev;
ptn_bridge->bridge.funcs = _bridge_funcs;
-   ptn_bridge->bridge.of_node = dev->of_node;
drm_bridge_add(_bridge->bridge);
 
i2c_set_clientdata(client, ptn_bridge);
-- 
2.11.0

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 09:17, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin <p...@axentia.se> wrote:
>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <p...@axentia.se> wrote:
>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <p...@axentia.se> wrote:
>>>>>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>>>>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and 
>>>>>>> msgbuf1,
>>>>>>> which are used to save a series of messages, as mentioned in the 
>>>>>>> comment.
>>>>>>> According to the value of the variable "size", msgbuf0 is initialized to
>>>>>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>>>>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>
>>>>>> double negation here
>>>>>>
>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 
>>>>>>> In
>>>>>>> the following execution, the uninitialized msgbuf1 will be used, such as
>>>>>>> for security checks. Since uninitialized values can be random and
>>>>>>> arbitrary, this will cause undefined behaviors or even check bypass. For
>>>>>>> example, it is expected that if the value of "size" is
>>>>>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be 
>>>>>>> larger
>>>>>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), 
>>>>>>> the
>>>>>>> value read from msgbuf1 is assigned to data->block[0], which can
>>>>>>> potentially lead to invalid block write size, as demonstrated in the 
>>>>>>> error
>>>>>>> message.
>>>>>>>
>>>>>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid 
>>>>>>> undefined
>>>>>>> behaviors or security issues.
>>>>>>>
>>>>>>> Signed-off-by: Wenwen Wang <wang6...@umn.edu>
>>>>>>> ---
>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>> index b5aec33..0fcca75 100644
>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct 
>>>>>>> i2c_adapter *adapter, u16 addr,
>>>>>>>* somewhat simpler.
>>>>>>>*/
>>>>>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>
>>>>>> I think this will result in the whole of msgbuf1 being filled with 
>>>>>> zeroes.
>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>> initializer?
>>>>>
>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>> used only in that case?
>>>>
>>>> I was thinking that an assignment of
>>>>
>>>> msgbuf1[0] = 0;
>>>>
>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>> seems like a non-issue to me.
>>>>
>>>> However, while looking I think the bigger problem with that function is 
>>>> that
>>>> it considers all non-negative

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 09:17, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 1:49 AM, Peter Rosin  wrote:
>> On 2018-05-04 07:28, Wenwen Wang wrote:
>>> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin  wrote:
>>>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin  wrote:
>>>>>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>>>>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and 
>>>>>>> msgbuf1,
>>>>>>> which are used to save a series of messages, as mentioned in the 
>>>>>>> comment.
>>>>>>> According to the value of the variable "size", msgbuf0 is initialized to
>>>>>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>>>>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>>>>>> initialized on all possible execution paths (implementation) of
>>>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>>>
>>>>>> double negation here
>>>>>>
>>>>>>> uninitialized even after the invocation of the function i2c_transfer(). 
>>>>>>> In
>>>>>>> the following execution, the uninitialized msgbuf1 will be used, such as
>>>>>>> for security checks. Since uninitialized values can be random and
>>>>>>> arbitrary, this will cause undefined behaviors or even check bypass. For
>>>>>>> example, it is expected that if the value of "size" is
>>>>>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be 
>>>>>>> larger
>>>>>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), 
>>>>>>> the
>>>>>>> value read from msgbuf1 is assigned to data->block[0], which can
>>>>>>> potentially lead to invalid block write size, as demonstrated in the 
>>>>>>> error
>>>>>>> message.
>>>>>>>
>>>>>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid 
>>>>>>> undefined
>>>>>>> behaviors or security issues.
>>>>>>>
>>>>>>> Signed-off-by: Wenwen Wang 
>>>>>>> ---
>>>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>>>> index b5aec33..0fcca75 100644
>>>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct 
>>>>>>> i2c_adapter *adapter, u16 addr,
>>>>>>>* somewhat simpler.
>>>>>>>*/
>>>>>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>>>
>>>>>> I think this will result in the whole of msgbuf1 being filled with 
>>>>>> zeroes.
>>>>>> It might be cheaper to do this with code proper rather than with an
>>>>>> initializer?
>>>>>
>>>>> Thanks for your comment, Peter!  How about using a memset() only when
>>>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>>>> used only in that case?
>>>>
>>>> I was thinking that an assignment of
>>>>
>>>> msgbuf1[0] = 0;
>>>>
>>>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>>>> cases before the i2c_transfer call. However, this will only kick in if
>>>> the call to kzalloc fails (and it most likely will not) in the call to the
>>>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>>>> seems like a non-issue to me.
>>>>
>>>> However, while looking I think the bigger problem with that function is 
>>>> that
>>>> it considers all non-negative return values from i2c_transfer as good.
>>>> IMHO, it should barf on any return values

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 07:28, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin <p...@axentia.se> wrote:
>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <p...@axentia.se> wrote:
>>>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
>>>>> which are used to save a series of messages, as mentioned in the comment.
>>>>> According to the value of the variable "size", msgbuf0 is initialized to
>>>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>>>> initialized on all possible execution paths (implementation) of
>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>
>>>> double negation here
>>>>
>>>>> uninitialized even after the invocation of the function i2c_transfer(). In
>>>>> the following execution, the uninitialized msgbuf1 will be used, such as
>>>>> for security checks. Since uninitialized values can be random and
>>>>> arbitrary, this will cause undefined behaviors or even check bypass. For
>>>>> example, it is expected that if the value of "size" is
>>>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be 
>>>>> larger
>>>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), 
>>>>> the
>>>>> value read from msgbuf1 is assigned to data->block[0], which can
>>>>> potentially lead to invalid block write size, as demonstrated in the error
>>>>> message.
>>>>>
>>>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>> behaviors or security issues.
>>>>>
>>>>> Signed-off-by: Wenwen Wang <wang6...@umn.edu>
>>>>> ---
>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>> index b5aec33..0fcca75 100644
>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
>>>>> *adapter, u16 addr,
>>>>>* somewhat simpler.
>>>>>*/
>>>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>
>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>> It might be cheaper to do this with code proper rather than with an
>>>> initializer?
>>>
>>> Thanks for your comment, Peter!  How about using a memset() only when
>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>> used only in that case?
>>
>> I was thinking that an assignment of
>>
>> msgbuf1[0] = 0;
>>
>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>> cases before the i2c_transfer call. However, this will only kick in if
>> the call to kzalloc fails (and it most likely will not) in the call to the
>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>> seems like a non-issue to me.
>>
>> However, while looking I think the bigger problem with that function is that
>> it considers all non-negative return values from i2c_transfer as good.
>> IMHO, it should barf on any return values <> num. Or at the very least
>> describe why a partial result is considered OK...
>>
>> Cheers,
>> Peter
>>
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>   int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>   int i;
>>>>>   u8 partial_pec = 0;
>>>>>
>>>>
>>
> 
> Yes, it is a big issue if the return value from i2c_transfer() is not
> equal to num. I can add a check like this:
> 
> if (status != num)
>   return -EINVAL;
> 

Right, but make sure to add it *after* the existing "if (status < 0)"
check as we want to preserve any existing error. Also, -EIO is perhaps
more appropriate than -EINVAL which seems wrong for what is probably
a runtime incident.

> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
> in i2c_smbus_try_get_dmabuf()?

It is not always replaced. The stack buffer is probably retained as
the default mode of operation (and fallback) because kzalloc is
expensive and because kzalloc might fail?

Cheers,
Peter

> Thanks,
> Wenwen
> 

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 07:28, Wenwen Wang wrote:
> On Fri, May 4, 2018 at 12:04 AM, Peter Rosin  wrote:
>> On 2018-05-04 06:08, Wenwen Wang wrote:
>>> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin  wrote:
>>>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
>>>>> which are used to save a series of messages, as mentioned in the comment.
>>>>> According to the value of the variable "size", msgbuf0 is initialized to
>>>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>>>> initialized on all possible execution paths (implementation) of
>>>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>>>
>>>> double negation here
>>>>
>>>>> uninitialized even after the invocation of the function i2c_transfer(). In
>>>>> the following execution, the uninitialized msgbuf1 will be used, such as
>>>>> for security checks. Since uninitialized values can be random and
>>>>> arbitrary, this will cause undefined behaviors or even check bypass. For
>>>>> example, it is expected that if the value of "size" is
>>>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be 
>>>>> larger
>>>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), 
>>>>> the
>>>>> value read from msgbuf1 is assigned to data->block[0], which can
>>>>> potentially lead to invalid block write size, as demonstrated in the error
>>>>> message.
>>>>>
>>>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>>>> behaviors or security issues.
>>>>>
>>>>> Signed-off-by: Wenwen Wang 
>>>>> ---
>>>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>>>> index b5aec33..0fcca75 100644
>>>>> --- a/drivers/i2c/i2c-core-smbus.c
>>>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
>>>>> *adapter, u16 addr,
>>>>>* somewhat simpler.
>>>>>*/
>>>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>>>
>>>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>>>> It might be cheaper to do this with code proper rather than with an
>>>> initializer?
>>>
>>> Thanks for your comment, Peter!  How about using a memset() only when
>>> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
>>> used only in that case?
>>
>> I was thinking that an assignment of
>>
>> msgbuf1[0] = 0;
>>
>> would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
>> cases before the i2c_transfer call. However, this will only kick in if
>> the call to kzalloc fails (and it most likely will not) in the call to the
>> i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
>> seems like a non-issue to me.
>>
>> However, while looking I think the bigger problem with that function is that
>> it considers all non-negative return values from i2c_transfer as good.
>> IMHO, it should barf on any return values <> num. Or at the very least
>> describe why a partial result is considered OK...
>>
>> Cheers,
>> Peter
>>
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>   int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>>>   int i;
>>>>>   u8 partial_pec = 0;
>>>>>
>>>>
>>
> 
> Yes, it is a big issue if the return value from i2c_transfer() is not
> equal to num. I can add a check like this:
> 
> if (status != num)
>   return -EINVAL;
> 

Right, but make sure to add it *after* the existing "if (status < 0)"
check as we want to preserve any existing error. Also, -EIO is perhaps
more appropriate than -EINVAL which seems wrong for what is probably
a runtime incident.

> Also, I wonder why msgbuf1 is necessary if it is replaced by kzalloc
> in i2c_smbus_try_get_dmabuf()?

It is not always replaced. The stack buffer is probably retained as
the default mode of operation (and fallback) because kzalloc is
expensive and because kzalloc might fail?

Cheers,
Peter

> Thanks,
> Wenwen
> 

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 06:08, Wenwen Wang wrote:
> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin <p...@axentia.se> wrote:
>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
>>> which are used to save a series of messages, as mentioned in the comment.
>>> According to the value of the variable "size", msgbuf0 is initialized to
>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>> initialized on all possible execution paths (implementation) of
>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>
>> double negation here
>>
>>> uninitialized even after the invocation of the function i2c_transfer(). In
>>> the following execution, the uninitialized msgbuf1 will be used, such as
>>> for security checks. Since uninitialized values can be random and
>>> arbitrary, this will cause undefined behaviors or even check bypass. For
>>> example, it is expected that if the value of "size" is
>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
>>> value read from msgbuf1 is assigned to data->block[0], which can
>>> potentially lead to invalid block write size, as demonstrated in the error
>>> message.
>>>
>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>> behaviors or security issues.
>>>
>>> Signed-off-by: Wenwen Wang <wang6...@umn.edu>
>>> ---
>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>> index b5aec33..0fcca75 100644
>>> --- a/drivers/i2c/i2c-core-smbus.c
>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
>>> *adapter, u16 addr,
>>>* somewhat simpler.
>>>*/
>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>
>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>> It might be cheaper to do this with code proper rather than with an
>> initializer?
> 
> Thanks for your comment, Peter!  How about using a memset() only when
> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
> used only in that case?

I was thinking that an assignment of

msgbuf1[0] = 0;

would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
cases before the i2c_transfer call. However, this will only kick in if
the call to kzalloc fails (and it most likely will not) in the call to the
i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
seems like a non-issue to me.

However, while looking I think the bigger problem with that function is that
it considers all non-negative return values from i2c_transfer as good.
IMHO, it should barf on any return values <> num. Or at the very least
describe why a partial result is considered OK...

Cheers,
Peter

>>
>> Cheers,
>> Peter
>>
>>>   int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>   int i;
>>>   u8 partial_pec = 0;
>>>
>>

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-04 06:08, Wenwen Wang wrote:
> On Thu, May 3, 2018 at 3:34 PM, Peter Rosin  wrote:
>> On 2018-05-03 00:36, Wenwen Wang wrote:
>>> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
>>> which are used to save a series of messages, as mentioned in the comment.
>>> According to the value of the variable "size", msgbuf0 is initialized to
>>> various values. In contrast, msgbuf1 is left uninitialized until the
>>> function i2c_transfer() is invoked. However, mgsbuf1 is not always
>>> initialized on all possible execution paths (implementation) of
>>> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be
>>
>> double negation here
>>
>>> uninitialized even after the invocation of the function i2c_transfer(). In
>>> the following execution, the uninitialized msgbuf1 will be used, such as
>>> for security checks. Since uninitialized values can be random and
>>> arbitrary, this will cause undefined behaviors or even check bypass. For
>>> example, it is expected that if the value of "size" is
>>> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
>>> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
>>> value read from msgbuf1 is assigned to data->block[0], which can
>>> potentially lead to invalid block write size, as demonstrated in the error
>>> message.
>>>
>>> This patch simply initializes the buffer msgbuf1 with 0 to avoid undefined
>>> behaviors or security issues.
>>>
>>> Signed-off-by: Wenwen Wang 
>>> ---
>>>  drivers/i2c/i2c-core-smbus.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>>> index b5aec33..0fcca75 100644
>>> --- a/drivers/i2c/i2c-core-smbus.c
>>> +++ b/drivers/i2c/i2c-core-smbus.c
>>> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
>>> *adapter, u16 addr,
>>>* somewhat simpler.
>>>*/
>>>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
>>> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
>>> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};
>>
>> I think this will result in the whole of msgbuf1 being filled with zeroes.
>> It might be cheaper to do this with code proper rather than with an
>> initializer?
> 
> Thanks for your comment, Peter!  How about using a memset() only when
> i2c_smbus_xfer_emulated() emulates reading commands, since msgbuf1 is
> used only in that case?

I was thinking that an assignment of

msgbuf1[0] = 0;

would be enough in the I2C_SMBUS_BLOCK_DATA and I2C_SMBUS_BLOCK_PROC_CALL
cases before the i2c_transfer call. However, this will only kick in if
the call to kzalloc fails (and it most likely will not) in the call to the
i2c_smbus_try_get_dmabuf helper. So, this thing that you are trying to fix
seems like a non-issue to me.

However, while looking I think the bigger problem with that function is that
it considers all non-negative return values from i2c_transfer as good.
IMHO, it should barf on any return values <> num. Or at the very least
describe why a partial result is considered OK...

Cheers,
Peter

>>
>> Cheers,
>> Peter
>>
>>>   int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>>>   int i;
>>>   u8 partial_pec = 0;
>>>
>>

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-03 11:06, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>> The more natural approach would perhaps be to add an drm_bridge_add,
>> but there are several other bridges that never call drm_bridge_add.
>> Just removing the drm_bridge_remove is the easier fix.
>>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
> 
> This mess is much bigger. There's 2 pairs of bridge functions:
> 
> - drm_bridge_attach/detach. Those are meant to be called by the overall
>   drm driver to connect/disconnect a drm_bridge.
> 
> - drm_bridge_add/remove. These are supposed to be called by the bridge
>   driver itself to register/unregister itself. Maybe we should rename
>   them, since the same issue happens with drm_panel, with the same
>   confusion.
> 
> I thought someone was working on a cleanup series to fix this mess, but I
> didn't find anything.

Ok, I just spotted the imbalance and didn't really dig into what
actually happens in these error paths. Now that I have done so I
believe that the removed drm_bridge_remove calls causes NULL
dereferences if/when the error paths are triggered.

So, I don't think this can wait for some bigger cleanup.

drm_bridge_remove calls list_del_init calls __list_del_entry calls
__list_del with NULL in both prev and next since the list member
is never initialized. prev and next are dereferenced by __list_del
and you have *boom*

I recommend adding the tag

Fixes: 84601dbdea36 ("drm: sti: rework init sequence")

so that stable picks this one up.

Cheers,
Peter

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>  2 files changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>> index 67bbdb49fffc..199db13f565c 100644
>> --- a/drivers/gpu/drm/sti/sti_hda.c
>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct 
>> device *master, void *data)
>>  return 0;
>>  
>>  err_sysfs:
>> -drm_bridge_remove(bridge);
>>  return -EINVAL;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>> index 58f431102512..932724784942 100644
>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct 
>> device *master, void *data)
>>  return 0;
>>  
>>  err_sysfs:
>> -drm_bridge_remove(bridge);
>>  hdmi->drm_connector = NULL;
>>  return -EINVAL;
>>  }
>> -- 
>> 2.11.0
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-03 11:06, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>> The more natural approach would perhaps be to add an drm_bridge_add,
>> but there are several other bridges that never call drm_bridge_add.
>> Just removing the drm_bridge_remove is the easier fix.
>>
>> Signed-off-by: Peter Rosin 
> 
> This mess is much bigger. There's 2 pairs of bridge functions:
> 
> - drm_bridge_attach/detach. Those are meant to be called by the overall
>   drm driver to connect/disconnect a drm_bridge.
> 
> - drm_bridge_add/remove. These are supposed to be called by the bridge
>   driver itself to register/unregister itself. Maybe we should rename
>   them, since the same issue happens with drm_panel, with the same
>   confusion.
> 
> I thought someone was working on a cleanup series to fix this mess, but I
> didn't find anything.

Ok, I just spotted the imbalance and didn't really dig into what
actually happens in these error paths. Now that I have done so I
believe that the removed drm_bridge_remove calls causes NULL
dereferences if/when the error paths are triggered.

So, I don't think this can wait for some bigger cleanup.

drm_bridge_remove calls list_del_init calls __list_del_entry calls
__list_del with NULL in both prev and next since the list member
is never initialized. prev and next are dereferenced by __list_del
and you have *boom*

I recommend adding the tag

Fixes: 84601dbdea36 ("drm: sti: rework init sequence")

so that stable picks this one up.

Cheers,
Peter

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>  2 files changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>> index 67bbdb49fffc..199db13f565c 100644
>> --- a/drivers/gpu/drm/sti/sti_hda.c
>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct 
>> device *master, void *data)
>>  return 0;
>>  
>>  err_sysfs:
>> -drm_bridge_remove(bridge);
>>  return -EINVAL;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>> index 58f431102512..932724784942 100644
>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct 
>> device *master, void *data)
>>  return 0;
>>  
>>  err_sysfs:
>> -drm_bridge_remove(bridge);
>>  hdmi->drm_connector = NULL;
>>  return -EINVAL;
>>  }
>> -- 
>> 2.11.0
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-03 00:36, Wenwen Wang wrote:
> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
> which are used to save a series of messages, as mentioned in the comment.
> According to the value of the variable "size", msgbuf0 is initialized to
> various values. In contrast, msgbuf1 is left uninitialized until the
> function i2c_transfer() is invoked. However, mgsbuf1 is not always
> initialized on all possible execution paths (implementation) of
> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be

double negation here

> uninitialized even after the invocation of the function i2c_transfer(). In
> the following execution, the uninitialized msgbuf1 will be used, such as
> for security checks. Since uninitialized values can be random and
> arbitrary, this will cause undefined behaviors or even check bypass. For
> example, it is expected that if the value of "size" is
> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
> value read from msgbuf1 is assigned to data->block[0], which can
> potentially lead to invalid block write size, as demonstrated in the error
> message.
> 
> This patch simply initializes the buffer msgbuf1 with 0 to avoid undefined
> behaviors or security issues.
> 
> Signed-off-by: Wenwen Wang 
> ---
>  drivers/i2c/i2c-core-smbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b5aec33..0fcca75 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>* somewhat simpler.
>*/
>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};

I think this will result in the whole of msgbuf1 being filled with zeroes.
It might be cheaper to do this with code proper rather than with an
initializer?

Cheers,
Peter

>   int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>   int i;
>   u8 partial_pec = 0;
> 

Re: [PATCH] i2c: core-smbus: fix a potential uninitialization bug

[Peter Rosin]

On 2018-05-03 00:36, Wenwen Wang wrote:
> In i2c_smbus_xfer_emulated(), there are two buffers: msgbuf0 and msgbuf1,
> which are used to save a series of messages, as mentioned in the comment.
> According to the value of the variable "size", msgbuf0 is initialized to
> various values. In contrast, msgbuf1 is left uninitialized until the
> function i2c_transfer() is invoked. However, mgsbuf1 is not always
> initialized on all possible execution paths (implementation) of
> i2c_transfer(). Thus, it is possible that mgsbuf1 may still not be

double negation here

> uninitialized even after the invocation of the function i2c_transfer(). In
> the following execution, the uninitialized msgbuf1 will be used, such as
> for security checks. Since uninitialized values can be random and
> arbitrary, this will cause undefined behaviors or even check bypass. For
> example, it is expected that if the value of "size" is
> I2C_SMBUS_BLOCK_PROC_CALL, the value of data->block[0] should not be larger
> than I2C_SMBUS_BLOCK_MAX. But, at the end of i2c_smbus_xfer_emulated(), the
> value read from msgbuf1 is assigned to data->block[0], which can
> potentially lead to invalid block write size, as demonstrated in the error
> message.
> 
> This patch simply initializes the buffer msgbuf1 with 0 to avoid undefined
> behaviors or security issues.
> 
> Signed-off-by: Wenwen Wang 
> ---
>  drivers/i2c/i2c-core-smbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b5aec33..0fcca75 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -324,7 +324,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
> *adapter, u16 addr,
>* somewhat simpler.
>*/
>   unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
> - unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
> + unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2] = {0};

I think this will result in the whole of msgbuf1 being filled with zeroes.
It might be cheaper to do this with code proper rather than with an
initializer?

Cheers,
Peter

>   int num = read_write == I2C_SMBUS_READ ? 2 : 1;
>   int i;
>   u8 partial_pec = 0;
> 

[PATCH 2/3] drm/rockchip: lvds: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index e67f4ea28c0e..4bd94b167d2c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -446,14 +446,12 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
goto err_free_connector;
}
} else {
-   lvds->bridge->encoder = encoder;
ret = drm_bridge_attach(encoder, lvds->bridge, NULL);
if (ret) {
DRM_DEV_ERROR(drm_dev->dev,
  "failed to attach bridge: %d\n", ret);
goto err_free_encoder;
}
-   encoder->bridge = lvds->bridge;
}
 
pm_runtime_enable(dev);
-- 
2.11.0

[PATCH 2/3] drm/rockchip: lvds: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c 
b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index e67f4ea28c0e..4bd94b167d2c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -446,14 +446,12 @@ static int rockchip_lvds_bind(struct device *dev, struct 
device *master,
goto err_free_connector;
}
} else {
-   lvds->bridge->encoder = encoder;
ret = drm_bridge_attach(encoder, lvds->bridge, NULL);
if (ret) {
DRM_DEV_ERROR(drm_dev->dev,
  "failed to attach bridge: %d\n", ret);
goto err_free_encoder;
}
-   encoder->bridge = lvds->bridge;
}
 
pm_runtime_enable(dev);
-- 
2.11.0

[PATCH 3/3] drm/exynos: hdmi: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c 
b/drivers/gpu/drm/exynos/exynos_hdmi.c
index abd84cbcf1c2..09c4bc0b1859 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -954,8 +954,6 @@ static int hdmi_create_connector(struct drm_encoder 
*encoder)
drm_mode_connector_attach_encoder(connector, encoder);
 
if (hdata->bridge) {
-   encoder->bridge = hdata->bridge;
-   hdata->bridge->encoder = encoder;
ret = drm_bridge_attach(encoder, hdata->bridge, NULL);
if (ret)
DRM_ERROR("Failed to attach bridge\n");
-- 
2.11.0

[PATCH 3/3] drm/exynos: hdmi: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin 
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c 
b/drivers/gpu/drm/exynos/exynos_hdmi.c
index abd84cbcf1c2..09c4bc0b1859 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -954,8 +954,6 @@ static int hdmi_create_connector(struct drm_encoder 
*encoder)
drm_mode_connector_attach_encoder(connector, encoder);
 
if (hdata->bridge) {
-   encoder->bridge = hdata->bridge;
-   hdata->bridge->encoder = encoder;
ret = drm_bridge_attach(encoder, hdata->bridge, NULL);
if (ret)
DRM_ERROR("Failed to attach bridge\n");
-- 
2.11.0

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

The more natural approach would perhaps be to add an drm_bridge_add,
but there are several other bridges that never call drm_bridge_add.
Just removing the drm_bridge_remove is the easier fix.

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/gpu/drm/sti/sti_hda.c  | 1 -
 drivers/gpu/drm/sti/sti_hdmi.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 67bbdb49fffc..199db13f565c 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device 
*master, void *data)
return 0;
 
 err_sysfs:
-   drm_bridge_remove(bridge);
return -EINVAL;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 58f431102512..932724784942 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct 
device *master, void *data)
return 0;
 
 err_sysfs:
-   drm_bridge_remove(bridge);
hdmi->drm_connector = NULL;
return -EINVAL;
 }
-- 
2.11.0

[PATCH 0/3] drm: fix some bridge api misunderstandings

[Peter Rosin]

Hi!

While looking at various drm bridge users, I came across these
issues.

Cheers,
Peter

Peter Rosin (3):
  drm/sti: do not remove the drm_bridge that was never added
  drm/rockchip: lvds: avoid duplicating drm_bridge_attach
  drm/exynos: hdmi: avoid duplicating drm_bridge_attach

 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 --
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 drivers/gpu/drm/sti/sti_hda.c| 1 -
 drivers/gpu/drm/sti/sti_hdmi.c   | 1 -
 4 files changed, 6 deletions(-)

-- 
2.11.0