* [<PATCH v1> 1/9] mmc: core: Add a cap to use long discard size
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-17 7:09 ` Ulf Hansson
2019-12-17 2:50 ` [<PATCH v1> 2/9] mmc: core: allow hosts to specify a large " Bao D. Nguyen
` (8 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Bao D. Nguyen
Add MMC_CAP2_MAX_DISCARD_SIZE cap which allows setting the max
discard size value if needed.
Setting a high value for the max discard size is to fix an issue where
some SD cards take a long time to perform the card format.
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
include/linux/mmc/host.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ba70338..f1a767d 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -348,6 +348,7 @@ struct mmc_host {
#define MMC_CAP2_FULL_PWR_CYCLE (1 << 2) /* Can do full power cycle */
#define MMC_CAP2_HS200_1_8V_SDR (1 << 5) /* can support */
#define MMC_CAP2_HS200_1_2V_SDR (1 << 6) /* can support */
+#define MMC_CAP2_MAX_DISCARD_SIZE (1 << 8) /* use max discard, ignoring max_busy_timeout parameter */
#define MMC_CAP2_HS200 (MMC_CAP2_HS200_1_8V_SDR | \
MMC_CAP2_HS200_1_2V_SDR)
#define MMC_CAP2_CD_ACTIVE_HIGH (1 << 10) /* Card-detect signal active high */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 1/9] mmc: core: Add a cap to use long discard size
2019-12-17 2:50 ` [<PATCH v1> 1/9] mmc: core: Add a cap to use long discard size Bao D. Nguyen
@ 2019-12-17 7:09 ` Ulf Hansson
0 siblings, 0 replies; 20+ messages in thread
From: Ulf Hansson @ 2019-12-17 7:09 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: Rob Herring, linux-mmc@vger.kernel.org, Linux Kernel Mailing List,
Asutosh Das, cang
On Tue, 17 Dec 2019 at 03:52, Bao D. Nguyen <nguyenb@codeaurora.org> wrote:
>
> Add MMC_CAP2_MAX_DISCARD_SIZE cap which allows setting the max
> discard size value if needed.
> Setting a high value for the max discard size is to fix an issue where
> some SD cards take a long time to perform the card format.
Can you please explain elaborate on why the SD card takes a long time
to "format"? What goes on and what takes time, etc.
In any case, it looks wrong to add a host cap to solve this problem.
Kind regards
Uffe
>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
> include/linux/mmc/host.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index ba70338..f1a767d 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -348,6 +348,7 @@ struct mmc_host {
> #define MMC_CAP2_FULL_PWR_CYCLE (1 << 2) /* Can do full power cycle */
> #define MMC_CAP2_HS200_1_8V_SDR (1 << 5) /* can support */
> #define MMC_CAP2_HS200_1_2V_SDR (1 << 6) /* can support */
> +#define MMC_CAP2_MAX_DISCARD_SIZE (1 << 8) /* use max discard, ignoring max_busy_timeout parameter */
> #define MMC_CAP2_HS200 (MMC_CAP2_HS200_1_8V_SDR | \
> MMC_CAP2_HS200_1_2V_SDR)
> #define MMC_CAP2_CD_ACTIVE_HIGH (1 << 10) /* Card-detect signal active high */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 20+ messages in thread
* [<PATCH v1> 2/9] mmc: core: allow hosts to specify a large discard size
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
2019-12-17 2:50 ` [<PATCH v1> 1/9] mmc: core: Add a cap to use long discard size Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-17 2:50 ` [<PATCH v1> 3/9] mmc: host: Add device_prepare pm for mmc_host Bao D. Nguyen
` (7 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Ziqi Chen,
Sayali Lokhande, Bao D. Nguyen
From: Ziqi Chen <ziqichen@codeaurora.org>
max_busy_timeout is used to decide whether R1B response should
be used or a R1 response should be used. This is also used to
decide what the discard size of mmc queue (registered with block
layer) can be set to. In order to keep both the features in place,
this change will allow for hosts to specify a larger discard size
while still specifying max_busy_timeout.
Signed-off-by: Ziqi Chen <ziqichen@codeaurora.org>
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/core/core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index abf8f5e..1e37f78 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2059,6 +2059,10 @@ unsigned int mmc_calc_max_discard(struct mmc_card *card)
struct mmc_host *host = card->host;
unsigned int max_discard, max_trim;
+ if (!host->max_busy_timeout ||
+ (host->caps2 & MMC_CAP2_MAX_DISCARD_SIZE))
+ return UINT_MAX;
+
/*
* Without erase_group_def set, MMC erase timeout depends on clock
* frequence which can change. In that case, the best choice is
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* [<PATCH v1> 3/9] mmc: host: Add device_prepare pm for mmc_host
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
2019-12-17 2:50 ` [<PATCH v1> 1/9] mmc: core: Add a cap to use long discard size Bao D. Nguyen
2019-12-17 2:50 ` [<PATCH v1> 2/9] mmc: core: allow hosts to specify a large " Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-17 2:50 ` [<PATCH v1> 4/9] mmc: core: fix SD card request queue refcount underflow during shutdown Bao D. Nguyen
` (6 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Vijay Viswanath,
Ram Prakash Gupta, Bao D. Nguyen
From: Vijay Viswanath <vviswana@codeaurora.org>
mmc_host is a virtual device and it doesn't have any pm ops and so during
pm registration of device, no_pm_callback gets set as true. The
mmc_host device is not runtime enabled as it is a virtual device and
mmc_host is the parent device of mmc_card. As the mmc_host is runtime
disabled, mmc_card can runtime suspend/resume without depending on
state of mmc_host during normal operations. During system suspend, the
direct_complete flag of mmc_host device gets set as it has no pm_ops.
When mmc_card successfully suspends, it clears the direct_complete flag
of its parent (mmc_host).
But in certain cases during dpm_suspend, an async error can occur after
suspend work for mmc_card is scheduled and before it gets executed. In
that case, mmc_card suspend work will not clear the direct_complete flag
of mmc_host. When mmc_host suspend comes after that of mmc_card,
it too will skip all actions.
But by this time, the mmc_host device has been added to device_suspended
list. So during resume, mmc_host resume will do dpm resume of mmc_host.
In dpm_resume, all devices which has direct_complete flag set will be
runtime_enabled. This is because, in dpm_suspend, any device with
direct_complete flag will be runtime_disabled. Thus, mmc_host which has
direct_complete flag set, will get runtime enabled during dpm_resume.
This is a problem in pm framework with direct_complete flag
(runtime enabling a device in resume when it was not runtime disabled
in suspend path).
Now that mmc_host device is runtime enabled, to runtime resume the
mmc_card, the pm framework will try to runtime resume the mmc_host
device as well and will fail. This prevents mmc_card from runtime
resuming after a runtime_suspend.
Fix this by adding a dummy suspend_prepare() fn for mmc_host. This
prevents the direct_complete flag of mmc_host device from getting set.
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Ram Prakash Gupta <rampraka@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/core/host.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 105b7a7..242657b 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -40,9 +40,28 @@ static void mmc_host_classdev_release(struct device *dev)
kfree(host);
}
+static int mmc_host_prepare(struct device *dev)
+{
+ /*
+ * Since mmc_host is a virtual device, we don't have to do anything.
+ * If we return a positive value, the pm framework will consider that
+ * the runtime suspend and system suspend of this device is same and
+ * will set direct_complete flag as true. We don't want this as the
+ * mmc_host always has positive disable_depth and setting the flag
+ * will not speed up the suspend process.
+ * So return 0.
+ */
+ return 0;
+}
+
+static const struct dev_pm_ops mmc_pm_ops = {
+ .prepare = mmc_host_prepare,
+};
+
static struct class mmc_host_class = {
.name = "mmc_host",
.dev_release = mmc_host_classdev_release,
+ .pm = &mmc_pm_ops,
};
int mmc_register_host_class(void)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* [<PATCH v1> 4/9] mmc: core: fix SD card request queue refcount underflow during shutdown
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
` (2 preceding siblings ...)
2019-12-17 2:50 ` [<PATCH v1> 3/9] mmc: host: Add device_prepare pm for mmc_host Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-18 8:33 ` Greg KH
2019-12-17 2:50 ` [<PATCH v1> 5/9] mmc: core: fix one NULL pointer dereference after SD card is removed Bao D. Nguyen
` (5 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Sayali Lokhande,
Bao D. Nguyen
From: Can Guo <cang@codeaurora.org>
When system shutdown, kernel shall call shutdown function of mmc to stop
its request queue and clean it up, during which the request queue's kobject
shall be put once. In normal cases, if the SD card is removed, the
mmc_blk_remove routine releases all the resources and kobjects related to
the disk and request queue by decreasing their kref counts to 0. But if the
SD card is removed after its shutdown function is called, below kref count
underflow error shall be thrown out because the kref count was decreased
once during request queue cleanup by the shutdown function in advance. This
change fixes it by skipping request queue cleanup in the mmc blk routine if
the queue has been marked as dead.
[ 166.187211] refcount_t: underflow; use-after-free.
[ 166.187277] ------------[ cut here ]------------
[ 166.187321] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[ 166.187542] Workqueue: events_freezable mmc_rescan
[ 166.187558] task: ffffffe673b96680 task.stack: ffffff8008418000
[ 166.187579] pc : refcount_sub_and_test+0x64/0x78
[ 166.187593] lr : refcount_sub_and_test+0x64/0x78
[ 166.187605] sp : ffffff800841ba20 pstate : 60c00145
[ 166.188319] Call trace:
[ 166.188331] refcount_sub_and_test+0x64/0x78
[ 166.188343] refcount_dec_and_test+0x18/0x24
[ 166.188355] kobject_put+0x5c/0x74
[ 166.188374] blk_put_queue+0x1c/0x28
[ 166.188388] disk_release+0x70/0x90
[ 166.188402] device_release+0x38/0x90
[ 166.188429] kobject_cleanup+0xc4/0x1c0
[ 166.188441] kobject_put+0x68/0x74
[ 166.188455] put_disk+0x20/0x2c
[ 166.188467] mmc_blk_put+0x9c/0xdc
[ 166.188480] mmc_blk_remove_req+0x110/0x120
[ 166.188493] mmc_blk_remove+0x14c/0x22c
[ 166.188505] mmc_bus_remove+0x24/0x34
[ 166.188517] device_release_driver_internal+0x13c/0x1e0
[ 166.188528] device_release_driver+0x24/0x30
[ 166.188540] bus_remove_device+0xdc/0x120
[ 166.188552] device_del+0x1e0/0x284
[ 166.188564] mmc_remove_card+0x68/0x7c
[ 166.188577] mmc_sd_remove+0x24/0x48
[ 166.188588] mmc_sd_detect+0x120/0x1a4
[ 166.188600] mmc_rescan+0xf4/0x384
[ 166.188613] process_one_work+0x1c0/0x3d4
[ 166.188625] worker_thread+0x224/0x344
[ 166.188637] kthread+0x120/0x130
[ 166.188649] ret_from_fork+0x10/0x18.
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/core/queue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 9edc086..846557b 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -506,7 +506,8 @@ void mmc_cleanup_queue(struct mmc_queue *mq)
if (blk_queue_quiesced(q))
blk_mq_unquiesce_queue(q);
- blk_cleanup_queue(q);
+ if (likely(!blk_queue_dead(q)))
+ blk_cleanup_queue(q);
blk_mq_free_tag_set(&mq->tag_set);
/*
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 4/9] mmc: core: fix SD card request queue refcount underflow during shutdown
2019-12-17 2:50 ` [<PATCH v1> 4/9] mmc: core: fix SD card request queue refcount underflow during shutdown Bao D. Nguyen
@ 2019-12-18 8:33 ` Greg KH
0 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2019-12-18 8:33 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: ulf.hansson, robh+dt, linux-mmc, linux-kernel, asutoshd, cang,
Sayali Lokhande
On Mon, Dec 16, 2019 at 06:50:37PM -0800, Bao D. Nguyen wrote:
> From: Can Guo <cang@codeaurora.org>
>
> When system shutdown, kernel shall call shutdown function of mmc to stop
> its request queue and clean it up, during which the request queue's kobject
> shall be put once. In normal cases, if the SD card is removed, the
> mmc_blk_remove routine releases all the resources and kobjects related to
> the disk and request queue by decreasing their kref counts to 0. But if the
> SD card is removed after its shutdown function is called, below kref count
> underflow error shall be thrown out because the kref count was decreased
> once during request queue cleanup by the shutdown function in advance. This
> change fixes it by skipping request queue cleanup in the mmc blk routine if
> the queue has been marked as dead.
>
> [ 166.187211] refcount_t: underflow; use-after-free.
> [ 166.187277] ------------[ cut here ]------------
> [ 166.187321] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> [ 166.187542] Workqueue: events_freezable mmc_rescan
> [ 166.187558] task: ffffffe673b96680 task.stack: ffffff8008418000
> [ 166.187579] pc : refcount_sub_and_test+0x64/0x78
> [ 166.187593] lr : refcount_sub_and_test+0x64/0x78
> [ 166.187605] sp : ffffff800841ba20 pstate : 60c00145
> [ 166.188319] Call trace:
> [ 166.188331] refcount_sub_and_test+0x64/0x78
> [ 166.188343] refcount_dec_and_test+0x18/0x24
> [ 166.188355] kobject_put+0x5c/0x74
> [ 166.188374] blk_put_queue+0x1c/0x28
> [ 166.188388] disk_release+0x70/0x90
> [ 166.188402] device_release+0x38/0x90
> [ 166.188429] kobject_cleanup+0xc4/0x1c0
> [ 166.188441] kobject_put+0x68/0x74
> [ 166.188455] put_disk+0x20/0x2c
> [ 166.188467] mmc_blk_put+0x9c/0xdc
> [ 166.188480] mmc_blk_remove_req+0x110/0x120
> [ 166.188493] mmc_blk_remove+0x14c/0x22c
> [ 166.188505] mmc_bus_remove+0x24/0x34
> [ 166.188517] device_release_driver_internal+0x13c/0x1e0
> [ 166.188528] device_release_driver+0x24/0x30
> [ 166.188540] bus_remove_device+0xdc/0x120
> [ 166.188552] device_del+0x1e0/0x284
> [ 166.188564] mmc_remove_card+0x68/0x7c
> [ 166.188577] mmc_sd_remove+0x24/0x48
> [ 166.188588] mmc_sd_detect+0x120/0x1a4
> [ 166.188600] mmc_rescan+0xf4/0x384
> [ 166.188613] process_one_work+0x1c0/0x3d4
> [ 166.188625] worker_thread+0x224/0x344
> [ 166.188637] kthread+0x120/0x130
> [ 166.188649] ret_from_fork+0x10/0x18.
>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
> drivers/mmc/core/queue.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index 9edc086..846557b 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -506,7 +506,8 @@ void mmc_cleanup_queue(struct mmc_queue *mq)
> if (blk_queue_quiesced(q))
> blk_mq_unquiesce_queue(q);
>
> - blk_cleanup_queue(q);
> + if (likely(!blk_queue_dead(q)))
> + blk_cleanup_queue(q);
Unless you can measure the performance impact, never use unlikely/likely
in kernel code. The compiler and cpu will always do much better over
time than you can.
That being said, what will cleanup the queue if it is not "dead" at this
point in time, later on? Isn't this a leak?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread
* [<PATCH v1> 5/9] mmc: core: fix one NULL pointer dereference after SD card is removed
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
` (3 preceding siblings ...)
2019-12-17 2:50 ` [<PATCH v1> 4/9] mmc: core: fix SD card request queue refcount underflow during shutdown Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-17 2:50 ` [<PATCH v1> 6/9] mmc: sdhci-msm: Ignore data timeout error for R1B commands Bao D. Nguyen
` (4 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Sayali Lokhande,
Bao D. Nguyen
From: Can Guo <cang@codeaurora.org>
After SD card is removed, the driver would mark its queue DYING to try to
block further more requests from coming into the queue, then clean up its
queue's queuedata by setting it to NULL. However, there can still be new
requests come in right before the DYING mark is set after SD card is
removed. When one new request is allocated and initialized, the queuedata
would be accessed. If queuedata has been cleaned up already, NULL pointer
dereference would happen. This change fixes it by checking if queuedata is
NULL before accessing it, if yes, then bails out with error.
mmc0: card aaaa removed
Buffer I/O error on dev mmcblk0p1, logical block 1, lost async page write
Unable to handle kernel NULL pointer dereference at virtual address
00000000
Mem abort info:
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000006
CM = 0, WnR = 0
user pgtable: 4k pages, 39-bit VAs, pgd = ffffffd7bbafa000
[0000000000000000] *pgd=0000000134331003, *pud=0000000134331003,
*pmd=0000000000000000
Internal error: Oops: 96000006 [#1] PREEMPT SMP
task: ffffffd77d193380 task.stack: ffffff8047e30000
pc : mmc_init_request+0x28/0x74
lr : alloc_request_size+0x4c/0x70
...
Process MediaScannerSer (pid: 4710, stack limit = 0xffffff8047e30000)
Call trace:
mmc_init_request+0x28/0x74
alloc_request_size+0x4c/0x70
mempool_alloc+0x104/0x184
get_request+0x324/0x75c
blk_queue_bio+0x154/0x398
generic_make_request+0xcc/0x228
submit_bio+0x13c/0x1d4.
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/core/queue.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 846557b..a1de5f7 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -211,8 +211,11 @@ static int __mmc_init_request(struct mmc_queue *mq, struct request *req,
gfp_t gfp)
{
struct mmc_queue_req *mq_rq = req_to_mmc_queue_req(req);
- struct mmc_card *card = mq->card;
- struct mmc_host *host = card->host;
+ struct mmc_host *host;
+
+ if (!mq)
+ return -ENODEV;
+ host = mq->card->host;
mq_rq->sg = mmc_alloc_sg(mmc_get_max_segments(host), gfp);
if (!mq_rq->sg)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* [<PATCH v1> 6/9] mmc: sdhci-msm: Ignore data timeout error for R1B commands
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
` (4 preceding siblings ...)
2019-12-17 2:50 ` [<PATCH v1> 5/9] mmc: core: fix one NULL pointer dereference after SD card is removed Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-18 8:34 ` Greg KH
2019-12-17 2:50 ` [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots Bao D. Nguyen
` (3 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Sahitya Tummala,
Bao D. Nguyen
From: Sahitya Tummala <stummala@codeaurora.org>
Ignore data timeout error for R1B commands as there will be no
data associated and the busy timeout value for these commands
could be lager than the maximum timeout value that controller
can handle.
Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/host/sdhci.c | 15 +++++++++------
drivers/mmc/host/sdhci.h | 7 +++++++
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c04e1ac..0a05d74 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2925,12 +2925,6 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
* above in sdhci_cmd_irq().
*/
if (data_cmd && (data_cmd->flags & MMC_RSP_BUSY)) {
- if (intmask & SDHCI_INT_DATA_TIMEOUT) {
- host->data_cmd = NULL;
- data_cmd->error = -ETIMEDOUT;
- __sdhci_finish_mrq(host, data_cmd->mrq);
- return;
- }
if (intmask & SDHCI_INT_DATA_END) {
host->data_cmd = NULL;
/*
@@ -2944,6 +2938,15 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
__sdhci_finish_mrq(host, data_cmd->mrq);
return;
}
+ if (host->quirks2 &
+ SDHCI_QUIRK2_IGNORE_DATATOUT_FOR_R1BCMD)
+ return;
+ if (intmask & SDHCI_INT_DATA_TIMEOUT) {
+ host->data_cmd = NULL;
+ data_cmd->error = -ETIMEDOUT;
+ __sdhci_finish_mrq(host, data_cmd->mrq);
+ return;
+ }
}
/*
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0ed3e0e..1a88f74 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -482,6 +482,13 @@ struct sdhci_host {
* block count.
*/
#define SDHCI_QUIRK2_USE_32BIT_BLK_CNT (1<<18)
+/*
+ * Ignore data timeout error for R1B commands as there will be no
+ * data associated and the busy timeout value for these commands
+ * could be lager than the maximum timeout value that controller
+ * can handle.
+ */
+#define SDHCI_QUIRK2_IGNORE_DATATOUT_FOR_R1BCMD (1<<19)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 6/9] mmc: sdhci-msm: Ignore data timeout error for R1B commands
2019-12-17 2:50 ` [<PATCH v1> 6/9] mmc: sdhci-msm: Ignore data timeout error for R1B commands Bao D. Nguyen
@ 2019-12-18 8:34 ` Greg KH
0 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2019-12-18 8:34 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: ulf.hansson, robh+dt, linux-mmc, linux-kernel, asutoshd, cang,
Sahitya Tummala
On Mon, Dec 16, 2019 at 06:50:39PM -0800, Bao D. Nguyen wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
>
> Ignore data timeout error for R1B commands as there will be no
> data associated and the busy timeout value for these commands
> could be lager than the maximum timeout value that controller
> can handle.
>
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
> drivers/mmc/host/sdhci.c | 15 +++++++++------
> drivers/mmc/host/sdhci.h | 7 +++++++
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c04e1ac..0a05d74 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2925,12 +2925,6 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
> * above in sdhci_cmd_irq().
> */
> if (data_cmd && (data_cmd->flags & MMC_RSP_BUSY)) {
> - if (intmask & SDHCI_INT_DATA_TIMEOUT) {
> - host->data_cmd = NULL;
> - data_cmd->error = -ETIMEDOUT;
> - __sdhci_finish_mrq(host, data_cmd->mrq);
> - return;
> - }
> if (intmask & SDHCI_INT_DATA_END) {
> host->data_cmd = NULL;
> /*
> @@ -2944,6 +2938,15 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
> __sdhci_finish_mrq(host, data_cmd->mrq);
> return;
> }
> + if (host->quirks2 &
> + SDHCI_QUIRK2_IGNORE_DATATOUT_FOR_R1BCMD)
> + return;
> + if (intmask & SDHCI_INT_DATA_TIMEOUT) {
> + host->data_cmd = NULL;
> + data_cmd->error = -ETIMEDOUT;
> + __sdhci_finish_mrq(host, data_cmd->mrq);
> + return;
> + }
> }
>
> /*
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0ed3e0e..1a88f74 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -482,6 +482,13 @@ struct sdhci_host {
> * block count.
> */
> #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT (1<<18)
> +/*
> + * Ignore data timeout error for R1B commands as there will be no
> + * data associated and the busy timeout value for these commands
> + * could be lager than the maximum timeout value that controller
> + * can handle.
> + */
> +#define SDHCI_QUIRK2_IGNORE_DATATOUT_FOR_R1BCMD (1<<19)
No tabs?
And what about using BIT(19) instead?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread
* [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
` (5 preceding siblings ...)
2019-12-17 2:50 ` [<PATCH v1> 6/9] mmc: sdhci-msm: Ignore data timeout error for R1B commands Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-18 8:34 ` Greg KH
2019-12-17 2:50 ` [<PATCH v1> 8/9] mmc: core: remove shutdown handler Bao D. Nguyen
` (2 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Sujith Reddy Thumma,
Subhash Jadavani, Xiaonian Wang, Bao D. Nguyen
From: Sujith Reddy Thumma <sthumma@codeaurora.org>
Qualcomm SDCC controller supports minimum SD clock frequency
which is required for card initialization. This information is
exported through platform data for each SDCC controller. There is
no need of retrying higher frequencies than the minimum supported
by controller for Qualcomm chipsets which inturn add delay in
detection process if there is no card during suspend/resume cycles.
Hence, skip multiple frequency retries.
Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/core/core.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 1e37f78..38b0cec 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
{
struct mmc_host *host =
container_of(work, struct mmc_host, detect.work);
- int i;
if (host->rescan_disable)
return;
@@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
mmc_release_host(host);
goto out;
}
-
- for (i = 0; i < ARRAY_SIZE(freqs); i++) {
- if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
- break;
- if (freqs[i] <= host->f_min)
- break;
- }
+ mmc_rescan_try_freq(host, host->f_min);
mmc_release_host(host);
out:
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots
2019-12-17 2:50 ` [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots Bao D. Nguyen
@ 2019-12-18 8:34 ` Greg KH
2019-12-18 11:48 ` Ulf Hansson
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2019-12-18 8:34 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: ulf.hansson, robh+dt, linux-mmc, linux-kernel, asutoshd, cang,
Sujith Reddy Thumma, Subhash Jadavani, Xiaonian Wang
On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> From: Sujith Reddy Thumma <sthumma@codeaurora.org>
>
> Qualcomm SDCC controller supports minimum SD clock frequency
> which is required for card initialization. This information is
> exported through platform data for each SDCC controller. There is
> no need of retrying higher frequencies than the minimum supported
> by controller for Qualcomm chipsets which inturn add delay in
> detection process if there is no card during suspend/resume cycles.
> Hence, skip multiple frequency retries.
>
> Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
> drivers/mmc/core/core.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 1e37f78..38b0cec 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
> {
> struct mmc_host *host =
> container_of(work, struct mmc_host, detect.work);
> - int i;
>
> if (host->rescan_disable)
> return;
> @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
> mmc_release_host(host);
> goto out;
> }
> -
> - for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> - if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> - break;
> - if (freqs[i] <= host->f_min)
> - break;
> - }
> + mmc_rescan_try_freq(host, host->f_min);
What about for non-qualcomm controllers? Did this just break their
functionality?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots
2019-12-18 8:34 ` Greg KH
@ 2019-12-18 11:48 ` Ulf Hansson
2019-12-18 12:04 ` Greg Kroah-Hartman
0 siblings, 1 reply; 20+ messages in thread
From: Ulf Hansson @ 2019-12-18 11:48 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: Rob Herring, linux-mmc@vger.kernel.org, Linux Kernel Mailing List,
Asutosh Das, cang, Sujith Reddy Thumma, Subhash Jadavani,
Xiaonian Wang, Greg Kroah-Hartman
On Wed, 18 Dec 2019 at 09:34, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> > From: Sujith Reddy Thumma <sthumma@codeaurora.org>
> >
> > Qualcomm SDCC controller supports minimum SD clock frequency
> > which is required for card initialization. This information is
> > exported through platform data for each SDCC controller. There is
> > no need of retrying higher frequencies than the minimum supported
> > by controller for Qualcomm chipsets which inturn add delay in
> > detection process if there is no card during suspend/resume cycles.
> > Hence, skip multiple frequency retries.
> >
> > Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > ---
> > drivers/mmc/core/core.c | 9 +--------
> > 1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 1e37f78..38b0cec 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
> > {
> > struct mmc_host *host =
> > container_of(work, struct mmc_host, detect.work);
> > - int i;
> >
> > if (host->rescan_disable)
> > return;
> > @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
> > mmc_release_host(host);
> > goto out;
> > }
> > -
> > - for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > - if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > - break;
> > - if (freqs[i] <= host->f_min)
> > - break;
> > - }
> > + mmc_rescan_try_freq(host, host->f_min);
>
> What about for non-qualcomm controllers? Did this just break their
> functionality?
Yes it does, obviously.
Greg, thanks for providing some valuable feedback for Bao for a couple
of the patches in this series.
I have also browsed through the series, but stopped providing feedback
after patch1, when I realized that these are all just downstream
vendor specific hacks.
Sure, I guess most of the patches can be reworked as upstreamable
solutions, but rather than looking at them all at once, please post
them separately. Additionally, in general I would appreciate more
details in the changelogs to fully understand the problems your are
solving.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots
2019-12-18 11:48 ` Ulf Hansson
@ 2019-12-18 12:04 ` Greg Kroah-Hartman
2019-12-18 13:12 ` Ulf Hansson
0 siblings, 1 reply; 20+ messages in thread
From: Greg Kroah-Hartman @ 2019-12-18 12:04 UTC (permalink / raw)
To: Ulf Hansson
Cc: Bao D. Nguyen, Rob Herring, linux-mmc@vger.kernel.org,
Linux Kernel Mailing List, Asutosh Das, cang, Sujith Reddy Thumma,
Subhash Jadavani, Xiaonian Wang
On Wed, Dec 18, 2019 at 12:48:20PM +0100, Ulf Hansson wrote:
> On Wed, 18 Dec 2019 at 09:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> > > From: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > >
> > > Qualcomm SDCC controller supports minimum SD clock frequency
> > > which is required for card initialization. This information is
> > > exported through platform data for each SDCC controller. There is
> > > no need of retrying higher frequencies than the minimum supported
> > > by controller for Qualcomm chipsets which inturn add delay in
> > > detection process if there is no card during suspend/resume cycles.
> > > Hence, skip multiple frequency retries.
> > >
> > > Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > > Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > ---
> > > drivers/mmc/core/core.c | 9 +--------
> > > 1 file changed, 1 insertion(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > index 1e37f78..38b0cec 100644
> > > --- a/drivers/mmc/core/core.c
> > > +++ b/drivers/mmc/core/core.c
> > > @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
> > > {
> > > struct mmc_host *host =
> > > container_of(work, struct mmc_host, detect.work);
> > > - int i;
> > >
> > > if (host->rescan_disable)
> > > return;
> > > @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
> > > mmc_release_host(host);
> > > goto out;
> > > }
> > > -
> > > - for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > > - if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > > - break;
> > > - if (freqs[i] <= host->f_min)
> > > - break;
> > > - }
> > > + mmc_rescan_try_freq(host, host->f_min);
> >
> > What about for non-qualcomm controllers? Did this just break their
> > functionality?
>
> Yes it does, obviously.
>
> Greg, thanks for providing some valuable feedback for Bao for a couple
> of the patches in this series.
>
> I have also browsed through the series, but stopped providing feedback
> after patch1, when I realized that these are all just downstream
> vendor specific hacks.
That's all kernel drivers are, vendor-specific quirks/hacks around
broken hardware :)
Splitting this out into logical fixes, like some of these are here, is
great. But breaking non-qualcomm hardware like this patch would do, is
obviously not ok.
> Sure, I guess most of the patches can be reworked as upstreamable
> solutions,
They have to be upstreamable, you don't want these in random vendor
trees as they go no where and atrophy and break users. We want them in
our main tree for everyone to use for the obvious reason that they are
needed to get real hardware working.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots
2019-12-18 12:04 ` Greg Kroah-Hartman
@ 2019-12-18 13:12 ` Ulf Hansson
0 siblings, 0 replies; 20+ messages in thread
From: Ulf Hansson @ 2019-12-18 13:12 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Bao D. Nguyen, Rob Herring, linux-mmc@vger.kernel.org,
Linux Kernel Mailing List, Asutosh Das, cang, Sujith Reddy Thumma,
Subhash Jadavani, Xiaonian Wang
On Wed, 18 Dec 2019 at 13:04, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Dec 18, 2019 at 12:48:20PM +0100, Ulf Hansson wrote:
> > On Wed, 18 Dec 2019 at 09:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Dec 16, 2019 at 06:50:40PM -0800, Bao D. Nguyen wrote:
> > > > From: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > > >
> > > > Qualcomm SDCC controller supports minimum SD clock frequency
> > > > which is required for card initialization. This information is
> > > > exported through platform data for each SDCC controller. There is
> > > > no need of retrying higher frequencies than the minimum supported
> > > > by controller for Qualcomm chipsets which inturn add delay in
> > > > detection process if there is no card during suspend/resume cycles.
> > > > Hence, skip multiple frequency retries.
> > > >
> > > > Signed-off-by: Sujith Reddy Thumma <sthumma@codeaurora.org>
> > > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > > > Signed-off-by: Xiaonian Wang <xiaonian@codeaurora.org>
> > > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > > ---
> > > > drivers/mmc/core/core.c | 9 +--------
> > > > 1 file changed, 1 insertion(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > > index 1e37f78..38b0cec 100644
> > > > --- a/drivers/mmc/core/core.c
> > > > +++ b/drivers/mmc/core/core.c
> > > > @@ -2281,7 +2281,6 @@ void mmc_rescan(struct work_struct *work)
> > > > {
> > > > struct mmc_host *host =
> > > > container_of(work, struct mmc_host, detect.work);
> > > > - int i;
> > > >
> > > > if (host->rescan_disable)
> > > > return;
> > > > @@ -2332,13 +2331,7 @@ void mmc_rescan(struct work_struct *work)
> > > > mmc_release_host(host);
> > > > goto out;
> > > > }
> > > > -
> > > > - for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > > > - if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > > > - break;
> > > > - if (freqs[i] <= host->f_min)
> > > > - break;
> > > > - }
> > > > + mmc_rescan_try_freq(host, host->f_min);
> > >
> > > What about for non-qualcomm controllers? Did this just break their
> > > functionality?
> >
> > Yes it does, obviously.
> >
> > Greg, thanks for providing some valuable feedback for Bao for a couple
> > of the patches in this series.
> >
> > I have also browsed through the series, but stopped providing feedback
> > after patch1, when I realized that these are all just downstream
> > vendor specific hacks.
>
> That's all kernel drivers are, vendor-specific quirks/hacks around
> broken hardware :)
>
> Splitting this out into logical fixes, like some of these are here, is
> great. But breaking non-qualcomm hardware like this patch would do, is
> obviously not ok.
>
> > Sure, I guess most of the patches can be reworked as upstreamable
> > solutions,
>
> They have to be upstreamable, you don't want these in random vendor
> trees as they go no where and atrophy and break users. We want them in
> our main tree for everyone to use for the obvious reason that they are
> needed to get real hardware working.
I fully agree and I am willing to help!
Just didn't want to waste my time reviewing all at once in great
detail - if it turns out that the submitter don't care to re-spin, as
that has happened before (not by Bao).
Kind regards
Uffe
^ permalink raw reply [flat|nested] 20+ messages in thread
* [<PATCH v1> 8/9] mmc: core: remove shutdown handler
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
` (6 preceding siblings ...)
2019-12-17 2:50 ` [<PATCH v1> 7/9] mmc: core: Skip frequency retries for SDCC slots Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-17 2:50 ` [<PATCH v1> 9/9] mmc: sd: Fix trivial SD card issues Bao D. Nguyen
2019-12-18 8:21 ` [<PATCH v1> 0/9] SD card bug fixes Greg KH
9 siblings, 0 replies; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Subhash Jadavani,
Bao D. Nguyen
From: Subhash Jadavani <subhashj@codeaurora.org>
If mmc/sd shutdown callback suspends the card when card's runtime PM status
is still set to "runtime active" then any request issued after mmc/sd
shutdown will not resume the card hence we will end up issuing the request
to host driver while card isn't in a state to handle it.
Right way to fix this issue is not to allow any requests to flow in after
the shutdown callback has executed. Until it is fixed in the right way, we
are disabling the shutdown handling as it anyways doesn't do real useful
things.
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/core/mmc.c | 22 ----------------------
drivers/mmc/core/sd.c | 1 -
2 files changed, 23 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f6912de..a5110f7 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -2080,27 +2080,6 @@ static int _mmc_resume(struct mmc_host *host)
}
/*
- * Shutdown callback
- */
-static int mmc_shutdown(struct mmc_host *host)
-{
- int err = 0;
-
- /*
- * In a specific case for poweroff notify, we need to resume the card
- * before we can shutdown it properly.
- */
- if (mmc_can_poweroff_notify(host->card) &&
- !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
- err = _mmc_resume(host);
-
- if (!err)
- err = _mmc_suspend(host, false);
-
- return err;
-}
-
-/*
* Callback for resume.
*/
static int mmc_resume(struct mmc_host *host)
@@ -2185,7 +2164,6 @@ static int _mmc_hw_reset(struct mmc_host *host)
.runtime_suspend = mmc_runtime_suspend,
.runtime_resume = mmc_runtime_resume,
.alive = mmc_alive,
- .shutdown = mmc_shutdown,
.hw_reset = _mmc_hw_reset,
};
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index fe914ff..5938caf 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1259,7 +1259,6 @@ static int mmc_sd_hw_reset(struct mmc_host *host)
.suspend = mmc_sd_suspend,
.resume = mmc_sd_resume,
.alive = mmc_sd_alive,
- .shutdown = mmc_sd_suspend,
.hw_reset = mmc_sd_hw_reset,
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* [<PATCH v1> 9/9] mmc: sd: Fix trivial SD card issues
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
` (7 preceding siblings ...)
2019-12-17 2:50 ` [<PATCH v1> 8/9] mmc: core: remove shutdown handler Bao D. Nguyen
@ 2019-12-17 2:50 ` Bao D. Nguyen
2019-12-18 8:29 ` Greg KH
2019-12-18 8:21 ` [<PATCH v1> 0/9] SD card bug fixes Greg KH
9 siblings, 1 reply; 20+ messages in thread
From: Bao D. Nguyen @ 2019-12-17 2:50 UTC (permalink / raw)
To: ulf.hansson, robh+dt
Cc: linux-mmc, linux-kernel, asutoshd, cang, Bao D. Nguyen,
Bao D. Nguyen
From: "Bao D. Nguyen" <nguyenb@quicinc.com>
Fix various trivial SD card issues.
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
drivers/mmc/core/block.c | 4 ++--
drivers/mmc/core/bus.c | 13 +++++++++++++
drivers/mmc/core/core.c | 13 ++++++++-----
drivers/mmc/core/sd.c | 9 ++++++---
4 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 95b41c0..200882d 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -653,13 +653,13 @@ static int mmc_blk_ioctl_cmd(struct mmc_blk_data *md,
struct request *req;
idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
- if (IS_ERR(idata))
+ if (IS_ERR_OR_NULL(idata))
return PTR_ERR(idata);
/* This will be NULL on non-RPMB ioctl():s */
idata->rpmb = rpmb;
card = md->queue.card;
- if (IS_ERR(card)) {
+ if (IS_ERR_OR_NULL(card)) {
err = PTR_ERR(card);
goto cmd_done;
}
diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 74de3f2..fb17d21 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -131,6 +131,16 @@ static void mmc_bus_shutdown(struct device *dev)
struct mmc_host *host = card->host;
int ret;
+ if (!drv) {
+ pr_debug("%s: %s: drv is NULL\n", dev_name(dev), __func__);
+ return;
+ }
+
+ if (!card) {
+ pr_debug("%s: %s: card is NULL\n", dev_name(dev), __func__);
+ return;
+ }
+
if (dev->driver && drv->shutdown)
drv->shutdown(card);
@@ -247,12 +257,15 @@ void mmc_unregister_driver(struct mmc_driver *drv)
static void mmc_release_card(struct device *dev)
{
struct mmc_card *card = mmc_dev_to_card(dev);
+ struct mmc_host *host = card->host;
sdio_free_common_cis(card);
kfree(card->info);
kfree(card);
+ if (host)
+ host->card = NULL;
}
/*
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 38b0cec..13d496e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -399,7 +399,7 @@ void mmc_wait_for_req_done(struct mmc_host *host, struct mmc_request *mrq)
struct mmc_command *cmd;
while (1) {
- wait_for_completion(&mrq->completion);
+ wait_for_completion_io(&mrq->completion);
cmd = mrq->cmd;
@@ -666,6 +666,10 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
{
unsigned int mult;
+ if (!card) {
+ WARN_ON(1);
+ return;
+ }
/*
* SDIO cards only define an upper 1 s limit on access.
*/
@@ -2341,17 +2345,16 @@ void mmc_rescan(struct work_struct *work)
void mmc_start_host(struct mmc_host *host)
{
+ mmc_claim_host(host);
host->f_init = max(freqs[0], host->f_min);
host->rescan_disable = 0;
host->ios.power_mode = MMC_POWER_UNDEFINED;
- if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) {
- mmc_claim_host(host);
+ if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
mmc_power_up(host, host->ocr_avail);
- mmc_release_host(host);
- }
mmc_gpiod_request_cd_irq(host);
+ mmc_release_host(host);
_mmc_detect_change(host, 0, false);
}
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 5938caf..e163f0e 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -989,6 +989,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
err = mmc_send_relative_addr(host, &card->rca);
if (err)
goto free_card;
+ host->card = card;
}
if (!oldcard) {
@@ -1090,13 +1091,13 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
goto free_card;
}
done:
- host->card = card;
return 0;
free_card:
- if (!oldcard)
+ if (!oldcard) {
+ host->card = NULL;
mmc_remove_card(card);
-
+ }
return err;
}
@@ -1106,7 +1107,9 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
static void mmc_sd_remove(struct mmc_host *host)
{
mmc_remove_card(host->card);
+ mmc_claim_host(host);
host->card = NULL;
+ mmc_release_host(host);
}
/*
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 9/9] mmc: sd: Fix trivial SD card issues
2019-12-17 2:50 ` [<PATCH v1> 9/9] mmc: sd: Fix trivial SD card issues Bao D. Nguyen
@ 2019-12-18 8:29 ` Greg KH
2019-12-18 20:16 ` nguyenb
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2019-12-18 8:29 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: ulf.hansson, robh+dt, linux-mmc, linux-kernel, asutoshd, cang,
Bao D. Nguyen
On Mon, Dec 16, 2019 at 06:50:42PM -0800, Bao D. Nguyen wrote:
> From: "Bao D. Nguyen" <nguyenb@quicinc.com>
>
> Fix various trivial SD card issues.
There are a number of real bugfixes in here, please split these out and
put them at the beginning of the series so that they can be backported
to the stable kernel tree. Specifics below:
>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
> drivers/mmc/core/block.c | 4 ++--
> drivers/mmc/core/bus.c | 13 +++++++++++++
> drivers/mmc/core/core.c | 13 ++++++++-----
> drivers/mmc/core/sd.c | 9 ++++++---
> 4 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 95b41c0..200882d 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -653,13 +653,13 @@ static int mmc_blk_ioctl_cmd(struct mmc_blk_data *md,
> struct request *req;
>
> idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
> - if (IS_ERR(idata))
> + if (IS_ERR_OR_NULL(idata))
How can this function ever return NULL?
> return PTR_ERR(idata);
If NULL was returned, are you sure you can return 0 here? That implies
that all went well, when obviously it did not.
But again, I do not see how mmc_blk_ioctl_copy_from_user() can return
NULL, do you?
> /* This will be NULL on non-RPMB ioctl():s */
> idata->rpmb = rpmb;
>
> card = md->queue.card;
> - if (IS_ERR(card)) {
> + if (IS_ERR_OR_NULL(card)) {
How can card be NULL?
> err = PTR_ERR(card);
Again, returning "success" is ok? Are you sure?
> goto cmd_done;
> }
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 74de3f2..fb17d21 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -131,6 +131,16 @@ static void mmc_bus_shutdown(struct device *dev)
> struct mmc_host *host = card->host;
> int ret;
>
> + if (!drv) {
> + pr_debug("%s: %s: drv is NULL\n", dev_name(dev), __func__);
How can this ever happen?
And never use pr_* calls in a driver, you have a valid device, use
dev_dbg() and friends.
> + return;
> + }
> +
> + if (!card) {
> + pr_debug("%s: %s: card is NULL\n", dev_name(dev), __func__);
Same here, how can this ever happen?
> + return;
> + }
> +
> if (dev->driver && drv->shutdown)
> drv->shutdown(card);
>
> @@ -247,12 +257,15 @@ void mmc_unregister_driver(struct mmc_driver *drv)
> static void mmc_release_card(struct device *dev)
> {
> struct mmc_card *card = mmc_dev_to_card(dev);
> + struct mmc_host *host = card->host;
>
> sdio_free_common_cis(card);
>
> kfree(card->info);
>
> kfree(card);
> + if (host)
> + host->card = NULL;
Why are you setting this to null? Does this solve some race condition
that you are then catching in the shutdown callback? If so, this should
be broken out as a separate bugfix and put earlier in the series as that
should go to all stable kernels, right?
> }
>
> /*
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 38b0cec..13d496e 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -399,7 +399,7 @@ void mmc_wait_for_req_done(struct mmc_host *host, struct mmc_request *mrq)
> struct mmc_command *cmd;
>
> while (1) {
> - wait_for_completion(&mrq->completion);
> + wait_for_completion_io(&mrq->completion);
Why this change? That seems like a big one. Why is this not a separate
patch?
>
> cmd = mrq->cmd;
>
> @@ -666,6 +666,10 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
> {
> unsigned int mult;
>
> + if (!card) {
> + WARN_ON(1);
And you just crashed systems that run with panic-on-warn :(
How can this ever happen? If it is a real issue, catch it, log it, and
then move on, don't splat the kernel log with a full traceback and
reboot machines :(
> + return;
> + }
> /*
> * SDIO cards only define an upper 1 s limit on access.
> */
> @@ -2341,17 +2345,16 @@ void mmc_rescan(struct work_struct *work)
>
> void mmc_start_host(struct mmc_host *host)
> {
> + mmc_claim_host(host);
What? This is a totally separate change, plaese break this out and
describe what you are fixing here. Again, should be a bugfix for
earlier in the series.
> host->f_init = max(freqs[0], host->f_min);
> host->rescan_disable = 0;
> host->ios.power_mode = MMC_POWER_UNDEFINED;
>
> - if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) {
> - mmc_claim_host(host);
> + if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
> mmc_power_up(host, host->ocr_avail);
> - mmc_release_host(host);
> - }
>
> mmc_gpiod_request_cd_irq(host);
> + mmc_release_host(host);
And are you sure the reference counting is correct here? Before this
patch, you dropped the reference above, now you are matching it. Either
this is wrong, or the original code is wrong. Either way, you need to
describe it much better please.
> _mmc_detect_change(host, 0, false);
> }
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 5938caf..e163f0e 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -989,6 +989,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> err = mmc_send_relative_addr(host, &card->rca);
> if (err)
> goto free_card;
> + host->card = card;
Why?
> }
>
> if (!oldcard) {
> @@ -1090,13 +1091,13 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> goto free_card;
> }
> done:
> - host->card = card;
> return 0;
>
> free_card:
> - if (!oldcard)
> + if (!oldcard) {
> + host->card = NULL;
Again, why?
> mmc_remove_card(card);
> -
> + }
> return err;
> }
>
> @@ -1106,7 +1107,9 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> static void mmc_sd_remove(struct mmc_host *host)
> {
> mmc_remove_card(host->card);
> + mmc_claim_host(host);
> host->card = NULL;
> + mmc_release_host(host);
Huh? What is this "fixing"?
Again, please break all of these out into logical bugfixes and describe
what you are doing.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [<PATCH v1> 9/9] mmc: sd: Fix trivial SD card issues
2019-12-18 8:29 ` Greg KH
@ 2019-12-18 20:16 ` nguyenb
0 siblings, 0 replies; 20+ messages in thread
From: nguyenb @ 2019-12-18 20:16 UTC (permalink / raw)
To: Greg KH
Cc: ulf.hansson, robh+dt, linux-mmc, linux-kernel, asutoshd, cang,
Bao D. Nguyen
On 2019-12-18 00:29, Greg KH wrote:
> On Mon, Dec 16, 2019 at 06:50:42PM -0800, Bao D. Nguyen wrote:
>> From: "Bao D. Nguyen" <nguyenb@quicinc.com>
>>
>> Fix various trivial SD card issues.
>
> There are a number of real bugfixes in here, please split these out and
> put them at the beginning of the series so that they can be backported
> to the stable kernel tree. Specifics below:
>
>>
>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>> ---
>> drivers/mmc/core/block.c | 4 ++--
>> drivers/mmc/core/bus.c | 13 +++++++++++++
>> drivers/mmc/core/core.c | 13 ++++++++-----
>> drivers/mmc/core/sd.c | 9 ++++++---
>> 4 files changed, 29 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> index 95b41c0..200882d 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -653,13 +653,13 @@ static int mmc_blk_ioctl_cmd(struct mmc_blk_data
>> *md,
>> struct request *req;
>>
>> idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
>> - if (IS_ERR(idata))
>> + if (IS_ERR_OR_NULL(idata))
>
> How can this function ever return NULL?
>
>> return PTR_ERR(idata);
>
> If NULL was returned, are you sure you can return 0 here? That implies
> that all went well, when obviously it did not.
>
> But again, I do not see how mmc_blk_ioctl_copy_from_user() can return
> NULL, do you?
>
>> /* This will be NULL on non-RPMB ioctl():s */
>> idata->rpmb = rpmb;
>>
>> card = md->queue.card;
>> - if (IS_ERR(card)) {
>> + if (IS_ERR_OR_NULL(card)) {
>
> How can card be NULL?
>
>> err = PTR_ERR(card);
>
> Again, returning "success" is ok? Are you sure?
>
>> goto cmd_done;
>> }
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 74de3f2..fb17d21 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -131,6 +131,16 @@ static void mmc_bus_shutdown(struct device *dev)
>> struct mmc_host *host = card->host;
>> int ret;
>>
>> + if (!drv) {
>> + pr_debug("%s: %s: drv is NULL\n", dev_name(dev), __func__);
>
> How can this ever happen?
>
> And never use pr_* calls in a driver, you have a valid device, use
> dev_dbg() and friends.
>
>> + return;
>> + }
>> +
>> + if (!card) {
>> + pr_debug("%s: %s: card is NULL\n", dev_name(dev), __func__);
>
> Same here, how can this ever happen?
>
>> + return;
>> + }
>> +
>> if (dev->driver && drv->shutdown)
>> drv->shutdown(card);
>>
>> @@ -247,12 +257,15 @@ void mmc_unregister_driver(struct mmc_driver
>> *drv)
>> static void mmc_release_card(struct device *dev)
>> {
>> struct mmc_card *card = mmc_dev_to_card(dev);
>> + struct mmc_host *host = card->host;
>>
>> sdio_free_common_cis(card);
>>
>> kfree(card->info);
>>
>> kfree(card);
>> + if (host)
>> + host->card = NULL;
>
> Why are you setting this to null? Does this solve some race condition
> that you are then catching in the shutdown callback? If so, this
> should
> be broken out as a separate bugfix and put earlier in the series as
> that
> should go to all stable kernels, right?
>
>> }
>>
>> /*
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 38b0cec..13d496e 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -399,7 +399,7 @@ void mmc_wait_for_req_done(struct mmc_host *host,
>> struct mmc_request *mrq)
>> struct mmc_command *cmd;
>>
>> while (1) {
>> - wait_for_completion(&mrq->completion);
>> + wait_for_completion_io(&mrq->completion);
>
> Why this change? That seems like a big one. Why is this not a
> separate
> patch?
>
>>
>> cmd = mrq->cmd;
>>
>> @@ -666,6 +666,10 @@ void mmc_set_data_timeout(struct mmc_data *data,
>> const struct mmc_card *card)
>> {
>> unsigned int mult;
>>
>> + if (!card) {
>> + WARN_ON(1);
>
> And you just crashed systems that run with panic-on-warn :(
>
> How can this ever happen? If it is a real issue, catch it, log it, and
> then move on, don't splat the kernel log with a full traceback and
> reboot machines :(
>
>> + return;
>> + }
>> /*
>> * SDIO cards only define an upper 1 s limit on access.
>> */
>> @@ -2341,17 +2345,16 @@ void mmc_rescan(struct work_struct *work)
>>
>> void mmc_start_host(struct mmc_host *host)
>> {
>> + mmc_claim_host(host);
>
> What? This is a totally separate change, plaese break this out and
> describe what you are fixing here. Again, should be a bugfix for
> earlier in the series.
>
>> host->f_init = max(freqs[0], host->f_min);
>> host->rescan_disable = 0;
>> host->ios.power_mode = MMC_POWER_UNDEFINED;
>>
>> - if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) {
>> - mmc_claim_host(host);
>> + if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
>> mmc_power_up(host, host->ocr_avail);
>> - mmc_release_host(host);
>> - }
>>
>> mmc_gpiod_request_cd_irq(host);
>> + mmc_release_host(host);
>
> And are you sure the reference counting is correct here? Before this
> patch, you dropped the reference above, now you are matching it.
> Either
> this is wrong, or the original code is wrong. Either way, you need to
> describe it much better please.
>
>> _mmc_detect_change(host, 0, false);
>> }
>>
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 5938caf..e163f0e 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -989,6 +989,7 @@ static int mmc_sd_init_card(struct mmc_host *host,
>> u32 ocr,
>> err = mmc_send_relative_addr(host, &card->rca);
>> if (err)
>> goto free_card;
>> + host->card = card;
>
> Why?
>
>> }
>>
>> if (!oldcard) {
>> @@ -1090,13 +1091,13 @@ static int mmc_sd_init_card(struct mmc_host
>> *host, u32 ocr,
>> goto free_card;
>> }
>> done:
>> - host->card = card;
>> return 0;
>>
>> free_card:
>> - if (!oldcard)
>> + if (!oldcard) {
>> + host->card = NULL;
>
> Again, why?
>
>> mmc_remove_card(card);
>> -
>> + }
>> return err;
>> }
>>
>> @@ -1106,7 +1107,9 @@ static int mmc_sd_init_card(struct mmc_host
>> *host, u32 ocr,
>> static void mmc_sd_remove(struct mmc_host *host)
>> {
>> mmc_remove_card(host->card);
>> + mmc_claim_host(host);
>> host->card = NULL;
>> + mmc_release_host(host);
>
> Huh? What is this "fixing"?
>
> Again, please break all of these out into logical bugfixes and describe
> what you are doing.
>
> thanks,
>
> greg k-h
Thank you for providing the feedback. There are a lot of good feedback
and it will take me a bit of time to review them and make changes
properly.
Regards,
Bao
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [<PATCH v1> 0/9] SD card bug fixes
2019-12-17 2:50 [<PATCH v1> 0/9] SD card bug fixes Bao D. Nguyen
` (8 preceding siblings ...)
2019-12-17 2:50 ` [<PATCH v1> 9/9] mmc: sd: Fix trivial SD card issues Bao D. Nguyen
@ 2019-12-18 8:21 ` Greg KH
9 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2019-12-18 8:21 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: ulf.hansson, robh+dt, linux-mmc, linux-kernel, asutoshd, cang
On Mon, Dec 16, 2019 at 06:50:33PM -0800, Bao D. Nguyen wrote:
> These patches include SD card genernal bug fixes.
>
> Bao D. Nguyen (2):
> mmc: core: Add a cap to use long discard size
> mmc: sd: Fix trivial SD card issues
<snip>
Odd use of "<" and ">" in your subject lines, how did git create such a
thing?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread