Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-13 Thread Daniel Mack
Hi Grant,

On 13.10.2012 02:05, Grant Likely wrote:
 
 
 Balaji T K balaj...@ti.com wrote:
 
 On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:
 On 12.10.2012 16:56, Balaji T K wrote:
 On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
 On 12.10.2012 12:58, Daniel Mack wrote:
 diff --git a/drivers/mmc/host/omap_hsmmc.c
 b/drivers/mmc/host/omap_hsmmc.c
 index 19ccb59..4b70823 100644 ---
 a/drivers/mmc/host/omap_hsmmc.c +++
 b/drivers/mmc/host/omap_hsmmc.c @@ -1728,6 +1728,7 @@
 static int __devinit omap_hsmmc_probe(struct
 platform_device *pdev)
 const u16 *offsetp = match-data; pdata-reg_offset =
 *offsetp; } +pdev-dev.platform_data = pdata; }
 
 if (pdata == NULL) {
 
 
 FWIW, this is the Oops I see without this patch:
 Hi, Shouldn't pdev-dev.platform_data be set to NULL on _remove
 ?
 
 Why?
 
 To make sure on second insmod it is NULL, When built as module, So
 that of_get_hsmmc_pdata is called to create pdata.
 
 Actually the driver should *never* modify the value of
 dev-platform_data. Ever.

That's interesting, because many drivers do this, especially since they
were converted to DT probing. Mostly because that way, the platform data
logic in callback functions can be reused, and often the platform
specific data is only stored in pdata and taken from there during the
lifetime of a device.

Is there any particular reason why this approach is frowned upon?

 Make a copy instead.

A copy of what exactly? Of all members of the legacy pdata you mean?


Thanks,
Daniel

--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-13 Thread Grant Likely
On Sat, Oct 13, 2012 at 9:05 AM, Daniel Mack zon...@gmail.com wrote:
 Hi Grant,

 On 13.10.2012 02:05, Grant Likely wrote:


 Balaji T K balaj...@ti.com wrote:

 On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:
 On 12.10.2012 16:56, Balaji T K wrote:
 On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
 On 12.10.2012 12:58, Daniel Mack wrote:
 diff --git a/drivers/mmc/host/omap_hsmmc.c
 b/drivers/mmc/host/omap_hsmmc.c
 index 19ccb59..4b70823 100644 ---
 a/drivers/mmc/host/omap_hsmmc.c +++
 b/drivers/mmc/host/omap_hsmmc.c @@ -1728,6 +1728,7 @@
 static int __devinit omap_hsmmc_probe(struct
 platform_device *pdev)
 const u16 *offsetp = match-data; pdata-reg_offset =
 *offsetp; } +pdev-dev.platform_data = pdata; }

 if (pdata == NULL) {


 FWIW, this is the Oops I see without this patch:
 Hi, Shouldn't pdev-dev.platform_data be set to NULL on _remove
 ?

 Why?

 To make sure on second insmod it is NULL, When built as module, So
 that of_get_hsmmc_pdata is called to create pdata.

 Actually the driver should *never* modify the value of
 dev-platform_data. Ever.

 That's interesting, because many drivers do this, especially since they
 were converted to DT probing. Mostly because that way, the platform data
 logic in callback functions can be reused, and often the platform
 specific data is only stored in pdata and taken from there during the
 lifetime of a device.

 Is there any particular reason why this approach is frowned upon?

Yes. The platform data pointer is owned by the code that registered
the platform device, not by the device driver. Some drivers do it, but
it is definitely illegal. I should add code to the platform bus core
code to throw a warning to any drivers that do that. It is a problem
because it becomes easy to mess up the lifetime model of device data,
particularly when it comes to unbinding/rebinding devices. For
example, if a driver changes the pdata pointer and then gets unbound,
then there will be a stale pdata pointer that may point to either
incorrect data or a data structure that is no longer allocated. You
could argue that it is fine if the driver is smart about how it cleans
up after itself, but in my experience driver authors rarely get it
correct and it results in a lot more code than is necessary. It is far
better for the driver to either grab all the data it needs out of
pdata at .probe() time, or to keep a copy of the 'correct' pdata
(correct depending on where the device retrieved it's data) in it's
private data structure instead of modifying the device.


 Make a copy instead.

 A copy of what exactly? Of all members of the legacy pdata you mean?

Yes. If the driver directly accesses the pdata structure outside of
the .probe() hook, then it should be modified to either copy the pdata
into the driver's private data structure, or it should copy the
pointer to the pdata so that OF usage can allocate one itself. For
example:

somedriver_probe(struct platform_device *pdev)
{
  struct somedriver_private *somedriver;

  somedriver = devm_kzalloc(sizeof (*somedriver), GFP_KERNEL);
  somedriver-pdata = pdev-platform_data;
  if (OF)
  somedriver-pdata = devm_kzalloc(sizeof
(*somedriver-pdata), GFP_KERNEL);
}

The bonus with using devm_kzalloc is the driver doesn't even need to
do anything special to undo these allocations on failure or release.
:-)

g.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-13 Thread Daniel Mack
On 13.10.2012 10:48, Grant Likely wrote:
 On Sat, Oct 13, 2012 at 9:05 AM, Daniel Mack zon...@gmail.com wrote:
 Hi Grant,

 On 13.10.2012 02:05, Grant Likely wrote:


 Balaji T K balaj...@ti.com wrote:

 On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:
 On 12.10.2012 16:56, Balaji T K wrote:
 On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
 On 12.10.2012 12:58, Daniel Mack wrote:
 diff --git a/drivers/mmc/host/omap_hsmmc.c
 b/drivers/mmc/host/omap_hsmmc.c
 index 19ccb59..4b70823 100644 ---
 a/drivers/mmc/host/omap_hsmmc.c +++
 b/drivers/mmc/host/omap_hsmmc.c @@ -1728,6 +1728,7 @@
 static int __devinit omap_hsmmc_probe(struct
 platform_device *pdev)
 const u16 *offsetp = match-data; pdata-reg_offset =
 *offsetp; } +pdev-dev.platform_data = pdata; }

 if (pdata == NULL) {


 FWIW, this is the Oops I see without this patch:
 Hi, Shouldn't pdev-dev.platform_data be set to NULL on _remove
 ?

 Why?

 To make sure on second insmod it is NULL, When built as module, So
 that of_get_hsmmc_pdata is called to create pdata.

 Actually the driver should *never* modify the value of
 dev-platform_data. Ever.

 That's interesting, because many drivers do this, especially since they
 were converted to DT probing. Mostly because that way, the platform data
 logic in callback functions can be reused, and often the platform
 specific data is only stored in pdata and taken from there during the
 lifetime of a device.

 Is there any particular reason why this approach is frowned upon?
 
 Yes. The platform data pointer is owned by the code that registered
 the platform device, not by the device driver. Some drivers do it, but
 it is definitely illegal. I should add code to the platform bus core
 code to throw a warning to any drivers that do that. It is a problem
 because it becomes easy to mess up the lifetime model of device data,
 particularly when it comes to unbinding/rebinding devices. For
 example, if a driver changes the pdata pointer and then gets unbound,
 then there will be a stale pdata pointer that may point to either
 incorrect data or a data structure that is no longer allocated. You
 could argue that it is fine if the driver is smart about how it cleans
 up after itself, but in my experience driver authors rarely get it
 correct and it results in a lot more code than is necessary. It is far
 better for the driver to either grab all the data it needs out of
 pdata at .probe() time, or to keep a copy of the 'correct' pdata
 (correct depending on where the device retrieved it's data) in it's
 private data structure instead of modifying the device.

Ok, interesting. Thanks for writing this up.

 Make a copy instead.

 A copy of what exactly? Of all members of the legacy pdata you mean?
 
 Yes. If the driver directly accesses the pdata structure outside of
 the .probe() hook, then it should be modified to either copy the pdata
 into the driver's private data structure, or it should copy the
 pointer to the pdata so that OF usage can allocate one itself. For
 example:
 
 somedriver_probe(struct platform_device *pdev)
 {
   struct somedriver_private *somedriver;
 
   somedriver = devm_kzalloc(sizeof (*somedriver), GFP_KERNEL);
   somedriver-pdata = pdev-platform_data;
   if (OF)
   somedriver-pdata = devm_kzalloc(sizeof
 (*somedriver-pdata), GFP_KERNEL);
 }
 
 The bonus with using devm_kzalloc is the driver doesn't even need to
 do anything special to undo these allocations on failure or release.
 :-)

Ok, understood. Will keep an eye on this in the future. Thanks again for
the explanation.

For this particular driver, this means that both my and Balaji's ways of
fixing this are wrong?


Daniel

--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-13 Thread Grant Likely


Daniel Mack zon...@gmail.com wrote:

On 13.10.2012 10:48, Grant Likely wrote:
 
 somedriver_probe(struct platform_device *pdev)
 {
   struct somedriver_private *somedriver;
 
   somedriver = devm_kzalloc(sizeof (*somedriver), GFP_KERNEL);
   somedriver-pdata = pdev-platform_data;
   if (OF)
   somedriver-pdata = devm_kzalloc(sizeof
 (*somedriver-pdata), GFP_KERNEL);
 }
 
 The bonus with using devm_kzalloc is the driver doesn't even need to
 do anything special to undo these allocations on failure or release.
 :-)

Ok, understood. Will keep an eye on this in the future. Thanks again
for
the explanation.

For this particular driver, this means that both my and Balaji's ways
of
fixing this are wrong?

Balaji's patch looks fine since it uses a local copy of the platform data 
structure. It appears that the driver is already creating a local copy and 
Balaji is just bug fixing call sites that aren't using it yet.

g.

-- 
Grant Likely, P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-12 Thread Daniel Mack
On 12.10.2012 12:58, Daniel Mack wrote:
 When probed from DT, the self-allocated platform data has to be attached
 to the actual device. Otherwise a NULL pointer will be dereferenced from
 omap_hsmmc_card_detect if a gpio handle for card-detect has been passed.
 
 Signed-off-by: Daniel Mack zon...@gmail.com
 Cc: Venkatraman S svenk...@ti.com
 Cc: Chris Ball c...@laptop.org
 Cc: Grant Likely grant.lik...@secretlab.ca
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: linux-o...@vger.kernel.org
 ---
  drivers/mmc/host/omap_hsmmc.c | 1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
 index 19ccb59..4b70823 100644
 --- a/drivers/mmc/host/omap_hsmmc.c
 +++ b/drivers/mmc/host/omap_hsmmc.c
 @@ -1728,6 +1728,7 @@ static int __devinit omap_hsmmc_probe(struct 
 platform_device *pdev)
   const u16 *offsetp = match-data;
   pdata-reg_offset = *offsetp;
   }
 + pdev-dev.platform_data = pdata;
   }
  
   if (pdata == NULL) {
 

FWIW, this is the Oops I see without this patch:

[1.492727] Unable to handle kernel NULL pointer dereference at
virtual address 0040
[1.501327] pgd = c0004000
[1.504166] [0040] *pgd=
[1.507993] Internal error: Oops: 5 [#1] SMP THUMB2
[1.513100] Modules linked in:
[1.516315] CPU: 0Not tainted  (3.6.0-10656-g4bc7e4d-dirty #75)
[1.522890] PC is at omap_hsmmc_card_detect+0x6/0x18
[1.528090] LR is at omap_hsmmc_get_cd+0x1d/0x24
[1.532929] pc : [c025a3ee]lr : [c0259fed]psr: 4133
[1.532929] sp : cf1b5e78  ip : 0c288145  fp : 0002
[1.544939] r10: cf1b4000  r9 : cf1b5eb8  r8 : 
[1.550408] r7 : cf1a4600  r6 : 6113  r5 : cf33ba98  r4 : cf33b800
[1.557240] r3 :   r2 :   r1 :   r0 : cf0d1c10
[1.564075] Flags: nZcv  IRQs on  FIQs on  Mode SVC_32  ISA Thumb
Segment kernel
[1.571906] Control: 50c5387d  Table: 80004019  DAC: 0015
[1.577921] Process kworker/u:1 (pid: 19, stack limit = 0xcf1b4240)
[1.584480] Stack: (0xcf1b5e78 to 0xcf1b6000)
[1.589045] 5e60:
   c025a3e9 c0259fed
[1.597611] 5e80: c0259fd1 c024f959 cf33bb0c cf1a5dc0 c05c87c0
c003a42f 0002 
[1.606176] 5ea0: c003a3d8 c005e181 cf1b35c0  
c024f829 c0adfe68 c0687048
[1.614741] 5ec0:  c0494f78 c05c8780 cf1a5dc0 c05c88e8
cf1b4000 c05c88f0 cf1a5dd0
[1.623306] 5ee0: c0531f40 c053d678 c05c8780 c003c2ab cf1b5f18
c005e935 c05c87c0 c0531f40
[1.631872] 5f00: c0531f40 c7e3f1f9 cf1b5f34 cf069e20 cf1b5f34
 cf1a5dc0 c003c191
[1.640436] 5f20:    c003f809 
  cf1a5dc0
[1.649001] 5f40:   dead4ead  
c05c8ef8  
[1.657565] 5f60: c0455c08 cf1b5f64 cf1b5f64  
dead4ead  
[1.666130] 5f80: c05c8ef8   c0455c08 cf1b5f90
cf1b5f90 cf069e20 c003f791
[1.674694] 5fa0:    c000d1e1 aaa3aaab
b1ba aae3ea32 aba2a3aa
[1.683260] 5fc0: baaababe bbbaaaba aa2b bb2e2aba aabaa2bb
aaabb2bb aaabbaf3 f3bb2aaa
[1.691824] 5fe0: aabaabba 23ae2beb a2afab2a b2a2ab2a 0013
aaabaaab bfaf a0a2
[1.700399] [c025a3ee] (omap_hsmmc_card_detect+0x6/0x18) from
[c0259fed] (omap_hsmmc_get_cd+0x1d/0x24)
[1.710528] [c0259fed] (omap_hsmmc_get_cd+0x1d/0x24) from
[c024f959] (mmc_rescan+0x131/0x1e4)
[1.719842] [c024f959] (mmc_rescan+0x131/0x1e4) from [c003a42f]
(process_one_work+0x137/0x37c)
[1.729234] [c003a42f] (process_one_work+0x137/0x37c) from
[c003c2ab] (worker_thread+0x11b/0x364)
[1.738899] [c003c2ab] (worker_thread+0x11b/0x364) from
[c003f809] (kthread+0x79/0x84)
[1.747572] [c003f809] (kthread+0x79/0x84) from [c000d1e1]
(ret_from_kernel_thread+0xd/0x14)
[1.756773] Code: bf00 b508 f8d0 3084 (6c18) f75f
[1.761943] ---[ end trace b94c447c4835422d ]---

--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-12 Thread Balaji T K

On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:

On 12.10.2012 12:58, Daniel Mack wrote:

When probed from DT, the self-allocated platform data has to be attached
to the actual device. Otherwise a NULL pointer will be dereferenced from
omap_hsmmc_card_detect if a gpio handle for card-detect has been passed.

Signed-off-by: Daniel Mack zon...@gmail.com
Cc: Venkatraman S svenk...@ti.com
Cc: Chris Ball c...@laptop.org
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Rob Herring rob.herr...@calxeda.com
Cc: linux-o...@vger.kernel.org
---
  drivers/mmc/host/omap_hsmmc.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 19ccb59..4b70823 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1728,6 +1728,7 @@ static int __devinit omap_hsmmc_probe(struct 
platform_device *pdev)
const u16 *offsetp = match-data;
pdata-reg_offset = *offsetp;
}
+   pdev-dev.platform_data = pdata;
}

if (pdata == NULL) {



FWIW, this is the Oops I see without this patch:

Hi,
Shouldn't pdev-dev.platform_data be set to NULL on _remove ?

BTW, I posted a patch for the same by accessing saved version from 
host-pdata

http://permalink.gmane.org/gmane.linux.kernel.mmc/16996




[1.492727] Unable to handle kernel NULL pointer dereference at
virtual address 0040
[1.501327] pgd = c0004000
[1.504166] [0040] *pgd=
[1.507993] Internal error: Oops: 5 [#1] SMP THUMB2
[1.513100] Modules linked in:
[1.516315] CPU: 0Not tainted  (3.6.0-10656-g4bc7e4d-dirty #75)
[1.522890] PC is at omap_hsmmc_card_detect+0x6/0x18
[1.528090] LR is at omap_hsmmc_get_cd+0x1d/0x24
[1.532929] pc : [c025a3ee]lr : [c0259fed]psr: 4133
[1.532929] sp : cf1b5e78  ip : 0c288145  fp : 0002
[1.544939] r10: cf1b4000  r9 : cf1b5eb8  r8 : 
[1.550408] r7 : cf1a4600  r6 : 6113  r5 : cf33ba98  r4 : cf33b800
[1.557240] r3 :   r2 :   r1 :   r0 : cf0d1c10
[1.564075] Flags: nZcv  IRQs on  FIQs on  Mode SVC_32  ISA Thumb
Segment kernel
[1.571906] Control: 50c5387d  Table: 80004019  DAC: 0015
[1.577921] Process kworker/u:1 (pid: 19, stack limit = 0xcf1b4240)
[1.584480] Stack: (0xcf1b5e78 to 0xcf1b6000)
[1.589045] 5e60:
c025a3e9 c0259fed
[1.597611] 5e80: c0259fd1 c024f959 cf33bb0c cf1a5dc0 c05c87c0
c003a42f 0002 
[1.606176] 5ea0: c003a3d8 c005e181 cf1b35c0  
c024f829 c0adfe68 c0687048
[1.614741] 5ec0:  c0494f78 c05c8780 cf1a5dc0 c05c88e8
cf1b4000 c05c88f0 cf1a5dd0
[1.623306] 5ee0: c0531f40 c053d678 c05c8780 c003c2ab cf1b5f18
c005e935 c05c87c0 c0531f40
[1.631872] 5f00: c0531f40 c7e3f1f9 cf1b5f34 cf069e20 cf1b5f34
 cf1a5dc0 c003c191
[1.640436] 5f20:    c003f809 
  cf1a5dc0
[1.649001] 5f40:   dead4ead  
c05c8ef8  
[1.657565] 5f60: c0455c08 cf1b5f64 cf1b5f64  
dead4ead  
[1.666130] 5f80: c05c8ef8   c0455c08 cf1b5f90
cf1b5f90 cf069e20 c003f791
[1.674694] 5fa0:    c000d1e1 aaa3aaab
b1ba aae3ea32 aba2a3aa
[1.683260] 5fc0: baaababe bbbaaaba aa2b bb2e2aba aabaa2bb
aaabb2bb aaabbaf3 f3bb2aaa
[1.691824] 5fe0: aabaabba 23ae2beb a2afab2a b2a2ab2a 0013
aaabaaab bfaf a0a2
[1.700399] [c025a3ee] (omap_hsmmc_card_detect+0x6/0x18) from
[c0259fed] (omap_hsmmc_get_cd+0x1d/0x24)
[1.710528] [c0259fed] (omap_hsmmc_get_cd+0x1d/0x24) from
[c024f959] (mmc_rescan+0x131/0x1e4)
[1.719842] [c024f959] (mmc_rescan+0x131/0x1e4) from [c003a42f]
(process_one_work+0x137/0x37c)
[1.729234] [c003a42f] (process_one_work+0x137/0x37c) from
[c003c2ab] (worker_thread+0x11b/0x364)
[1.738899] [c003c2ab] (worker_thread+0x11b/0x364) from
[c003f809] (kthread+0x79/0x84)
[1.747572] [c003f809] (kthread+0x79/0x84) from [c000d1e1]
(ret_from_kernel_thread+0xd/0x14)
[1.756773] Code: bf00 b508 f8d0 3084 (6c18) f75f
[1.761943] ---[ end trace b94c447c4835422d ]---

--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-12 Thread Daniel Mack
On 12.10.2012 16:56, Balaji T K wrote:
 On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
 On 12.10.2012 12:58, Daniel Mack wrote:
 When probed from DT, the self-allocated platform data has to be attached
 to the actual device. Otherwise a NULL pointer will be dereferenced from
 omap_hsmmc_card_detect if a gpio handle for card-detect has been passed.

 Signed-off-by: Daniel Mack zon...@gmail.com
 Cc: Venkatraman S svenk...@ti.com
 Cc: Chris Ball c...@laptop.org
 Cc: Grant Likely grant.lik...@secretlab.ca
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: linux-o...@vger.kernel.org
 ---
   drivers/mmc/host/omap_hsmmc.c | 1 +
   1 file changed, 1 insertion(+)

 diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
 index 19ccb59..4b70823 100644
 --- a/drivers/mmc/host/omap_hsmmc.c
 +++ b/drivers/mmc/host/omap_hsmmc.c
 @@ -1728,6 +1728,7 @@ static int __devinit omap_hsmmc_probe(struct 
 platform_device *pdev)
 const u16 *offsetp = match-data;
 pdata-reg_offset = *offsetp;
 }
 +   pdev-dev.platform_data = pdata;
 }

 if (pdata == NULL) {


 FWIW, this is the Oops I see without this patch:
 Hi,
 Shouldn't pdev-dev.platform_data be set to NULL on _remove ?

Why?

 BTW, I posted a patch for the same by accessing saved version from 
 host-pdata
 http://permalink.gmane.org/gmane.linux.kernel.mmc/16996

Ok, that's another solution. I thought about this too, but then chose
the easier way :) I don't care which patch is taken, as long as we have
a fix in mainline.


Thanks,
Daniel

--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-12 Thread Balaji T K

On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:

On 12.10.2012 16:56, Balaji T K wrote:

On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:

On 12.10.2012 12:58, Daniel Mack wrote:

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 19ccb59..4b70823 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1728,6 +1728,7 @@ static int __devinit omap_hsmmc_probe(struct 
platform_device *pdev)
const u16 *offsetp = match-data;
pdata-reg_offset = *offsetp;
}
+   pdev-dev.platform_data = pdata;
}

if (pdata == NULL) {



FWIW, this is the Oops I see without this patch:

Hi,
Shouldn't pdev-dev.platform_data be set to NULL on _remove ?


Why?


To make sure on second insmod it is NULL, When built as module,
So that of_get_hsmmc_pdata is called to create pdata.




BTW, I posted a patch for the same by accessing saved version from
host-pdata
http://permalink.gmane.org/gmane.linux.kernel.mmc/16996


Ok, that's another solution. I thought about this too, but then chose
the easier way :) I don't care which patch is taken, as long as we have
a fix in mainline.



Agree this patch is easiest :-)



Thanks,
Daniel


--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/4] MMC: omap_hsmmc: set platform data after probe from DT node

2012-10-12 Thread Grant Likely


Balaji T K balaj...@ti.com wrote:

On Friday 12 October 2012 08:44 PM, Daniel Mack wrote:
 On 12.10.2012 16:56, Balaji T K wrote:
 On Friday 12 October 2012 07:59 PM, Daniel Mack wrote:
 On 12.10.2012 12:58, Daniel Mack wrote:
 diff --git a/drivers/mmc/host/omap_hsmmc.c
b/drivers/mmc/host/omap_hsmmc.c
 index 19ccb59..4b70823 100644
 --- a/drivers/mmc/host/omap_hsmmc.c
 +++ b/drivers/mmc/host/omap_hsmmc.c
 @@ -1728,6 +1728,7 @@ static int __devinit omap_hsmmc_probe(struct
platform_device *pdev)
   const u16 *offsetp = match-data;
   pdata-reg_offset = *offsetp;
   }
 + pdev-dev.platform_data = pdata;
   }

   if (pdata == NULL) {


 FWIW, this is the Oops I see without this patch:
 Hi,
 Shouldn't pdev-dev.platform_data be set to NULL on _remove ?

 Why?

To make sure on second insmod it is NULL, When built as module,
So that of_get_hsmmc_pdata is called to create pdata.

Actually the driver should *never* modify the value of dev-platform_data. 
Ever. Make a copy instead.

g.

-- 
Grant Likely, P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html