Re: [PATCH v9 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

[Ricky Liang]

On Wed, Dec 14, 2016 at 4:04 PM, Rick Chang <rick.ch...@mediatek.com> wrote:
> Add v4l2 driver for Mediatek JPEG Decoder
>
> Signed-off-by: Rick Chang <rick.ch...@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.t...@mediatek.com>
> ---
>  drivers/media/platform/Kconfig   |   15 +
>  drivers/media/platform/Makefile  |2 +
>  drivers/media/platform/mtk-jpeg/Makefile |2 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c  | 1306 
> ++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h  |  139 +++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c|  417 +++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h|   91 ++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.c |  160 +++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.h |   25 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_reg.h   |   58 +
>  10 files changed, 2215 insertions(+)

Reviewed-by: Ricky Liang <jcli...@chromium.org>
Tested-by: Ricky Liang <jcli...@chromium.org>

Re: [PATCH v9 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

[Ricky Liang]

On Wed, Dec 14, 2016 at 4:04 PM, Rick Chang  wrote:
> Add v4l2 driver for Mediatek JPEG Decoder
>
> Signed-off-by: Rick Chang 
> Signed-off-by: Minghsiu Tsai 
> ---
>  drivers/media/platform/Kconfig   |   15 +
>  drivers/media/platform/Makefile  |2 +
>  drivers/media/platform/mtk-jpeg/Makefile |2 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c  | 1306 
> ++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h  |  139 +++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c|  417 +++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h|   91 ++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.c |  160 +++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.h |   25 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_reg.h   |   58 +
>  10 files changed, 2215 insertions(+)

Reviewed-by: Ricky Liang 
Tested-by: Ricky Liang 

Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

[Ricky Liang]

Hi Rick,

Can you upload patchset v9 to address the issue? Thanks!

On Mon, Dec 12, 2016 at 5:07 PM, Rick Chang <rick.ch...@mediatek.com> wrote:
> Hi Ricky,
>
> Thanks for your feedback. We will fix the problem in another patch.
>
> On Mon, 2016-12-12 at 12:34 +0800, Ricky Liang wrote:
>> Hi Rick,
>>
>> On Wed, Nov 30, 2016 at 11:08 AM, Rick Chang <rick.ch...@mediatek.com> wrote:
>> > Add v4l2 driver for Mediatek JPEG Decoder
>> >
>> > Signed-off-by: Rick Chang <rick.ch...@mediatek.com>
>> > Signed-off-by: Minghsiu Tsai <minghsiu.t...@mediatek.com>
>>
>> 
>>
>> > +static bool mtk_jpeg_check_resolution_change(struct mtk_jpeg_ctx *ctx,
>> > +struct mtk_jpeg_dec_param 
>> > *param)
>> > +{
>> > +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>> > +   struct mtk_jpeg_q_data *q_data;
>> > +
>> > +   q_data = >out_q;
>> > +   if (q_data->w != param->pic_w || q_data->h != param->pic_h) {
>> > +   v4l2_dbg(1, debug, >v4l2_dev, "Picture size 
>> > change\n");
>> > +   return true;
>> > +   }
>> > +
>> > +   q_data = >cap_q;
>> > +   if (q_data->fmt != mtk_jpeg_find_format(ctx, param->dst_fourcc,
>> > +   
>> > MTK_JPEG_FMT_TYPE_CAPTURE)) {
>> > +   v4l2_dbg(1, debug, >v4l2_dev, "format change\n");
>> > +   return true;
>> > +   }
>> > +   return false;
>>
>> 
>>
>> > +static void mtk_jpeg_device_run(void *priv)
>> > +{
>> > +   struct mtk_jpeg_ctx *ctx = priv;
>> > +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>> > +   struct vb2_buffer *src_buf, *dst_buf;
>> > +   enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
>> > +   unsigned long flags;
>> > +   struct mtk_jpeg_src_buf *jpeg_src_buf;
>> > +   struct mtk_jpeg_bs bs;
>> > +   struct mtk_jpeg_fb fb;
>> > +   int i;
>> > +
>> > +   src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>> > +   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>> > +   jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(src_buf);
>> > +
>> > +   if (jpeg_src_buf->flags & MTK_JPEG_BUF_FLAGS_LAST_FRAME) {
>> > +   for (i = 0; i < dst_buf->num_planes; i++)
>> > +   vb2_set_plane_payload(dst_buf, i, 0);
>> > +   buf_state = VB2_BUF_STATE_DONE;
>> > +   goto dec_end;
>> > +   }
>> > +
>> > +   if (mtk_jpeg_check_resolution_change(ctx, 
>> > _src_buf->dec_param)) {
>> > +   mtk_jpeg_queue_src_chg_event(ctx);
>> > +   ctx->state = MTK_JPEG_SOURCE_CHANGE;
>> > +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>> > +   return;
>> > +   }
>>
>> This only detects source change if multiple OUPUT buffers are queued.
>> It does not catch the source change in the following scenario:
>>
>> - OUPUT buffers for jpeg1 enqueued
>> - OUTPUT queue STREAMON
>> - userspace creates CAPTURE buffers
>> - CAPTURE buffers enqueued
>> - CAPTURE queue STREAMON
>> - decode
>> - OUTPUT queue STREAMOFF
>> - userspace recreates OUTPUT buffers for jpeg2
>> - OUTPUT buffers for jpeg2 enqueued
>> - OUTPUT queue STREAMON
>>
>> In the above sequence if jpeg2's decoded size is larger than jpeg1 the
>> function fails to detect that the existing CAPTURE buffers are not big
>> enough to hold the decoded data.
>>
>> A possible fix is to pass *dst_buf to
>> mtk_jpeg_check_resolution_change(), and check in the function that all
>> the dst_buf planes are large enough to hold the decoded data.
>>
>> > +
>> > +   mtk_jpeg_set_dec_src(ctx, src_buf, );
>> > +   if (mtk_jpeg_set_dec_dst(ctx, _src_buf->dec_param, dst_buf, 
>> > ))
>> > +   goto dec_end;
>> > +
>> > +   spin_lock_irqsave(>hw_lock, flags);
>> > +   mtk_jpeg_dec_reset(jpeg->dec_reg_base);
>> > +   mtk_jpeg_dec_set_config(jpeg->dec_reg_base,
>> > +   _src_buf->dec_param, , );
>> > +
>> > +   mtk_jpeg_dec_start(jpeg->dec_reg_base);
>> > +   spin_unlock_irqrestore(>hw_lock, flags);
>> > +   return;
>> > +
>> > +dec_end:
>> > +   v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
>> > +   v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>> > +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf), buf_state);
>> > +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(dst_buf), buf_state);
>> > +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>> > +}
>>
>> 
>
>

Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

[Ricky Liang]

Hi Rick,

Can you upload patchset v9 to address the issue? Thanks!

On Mon, Dec 12, 2016 at 5:07 PM, Rick Chang  wrote:
> Hi Ricky,
>
> Thanks for your feedback. We will fix the problem in another patch.
>
> On Mon, 2016-12-12 at 12:34 +0800, Ricky Liang wrote:
>> Hi Rick,
>>
>> On Wed, Nov 30, 2016 at 11:08 AM, Rick Chang  wrote:
>> > Add v4l2 driver for Mediatek JPEG Decoder
>> >
>> > Signed-off-by: Rick Chang 
>> > Signed-off-by: Minghsiu Tsai 
>>
>> 
>>
>> > +static bool mtk_jpeg_check_resolution_change(struct mtk_jpeg_ctx *ctx,
>> > +struct mtk_jpeg_dec_param 
>> > *param)
>> > +{
>> > +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>> > +   struct mtk_jpeg_q_data *q_data;
>> > +
>> > +   q_data = >out_q;
>> > +   if (q_data->w != param->pic_w || q_data->h != param->pic_h) {
>> > +   v4l2_dbg(1, debug, >v4l2_dev, "Picture size 
>> > change\n");
>> > +   return true;
>> > +   }
>> > +
>> > +   q_data = >cap_q;
>> > +   if (q_data->fmt != mtk_jpeg_find_format(ctx, param->dst_fourcc,
>> > +   
>> > MTK_JPEG_FMT_TYPE_CAPTURE)) {
>> > +   v4l2_dbg(1, debug, >v4l2_dev, "format change\n");
>> > +   return true;
>> > +   }
>> > +   return false;
>>
>> 
>>
>> > +static void mtk_jpeg_device_run(void *priv)
>> > +{
>> > +   struct mtk_jpeg_ctx *ctx = priv;
>> > +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>> > +   struct vb2_buffer *src_buf, *dst_buf;
>> > +   enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
>> > +   unsigned long flags;
>> > +   struct mtk_jpeg_src_buf *jpeg_src_buf;
>> > +   struct mtk_jpeg_bs bs;
>> > +   struct mtk_jpeg_fb fb;
>> > +   int i;
>> > +
>> > +   src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>> > +   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>> > +   jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(src_buf);
>> > +
>> > +   if (jpeg_src_buf->flags & MTK_JPEG_BUF_FLAGS_LAST_FRAME) {
>> > +   for (i = 0; i < dst_buf->num_planes; i++)
>> > +   vb2_set_plane_payload(dst_buf, i, 0);
>> > +   buf_state = VB2_BUF_STATE_DONE;
>> > +   goto dec_end;
>> > +   }
>> > +
>> > +   if (mtk_jpeg_check_resolution_change(ctx, 
>> > _src_buf->dec_param)) {
>> > +   mtk_jpeg_queue_src_chg_event(ctx);
>> > +   ctx->state = MTK_JPEG_SOURCE_CHANGE;
>> > +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>> > +   return;
>> > +   }
>>
>> This only detects source change if multiple OUPUT buffers are queued.
>> It does not catch the source change in the following scenario:
>>
>> - OUPUT buffers for jpeg1 enqueued
>> - OUTPUT queue STREAMON
>> - userspace creates CAPTURE buffers
>> - CAPTURE buffers enqueued
>> - CAPTURE queue STREAMON
>> - decode
>> - OUTPUT queue STREAMOFF
>> - userspace recreates OUTPUT buffers for jpeg2
>> - OUTPUT buffers for jpeg2 enqueued
>> - OUTPUT queue STREAMON
>>
>> In the above sequence if jpeg2's decoded size is larger than jpeg1 the
>> function fails to detect that the existing CAPTURE buffers are not big
>> enough to hold the decoded data.
>>
>> A possible fix is to pass *dst_buf to
>> mtk_jpeg_check_resolution_change(), and check in the function that all
>> the dst_buf planes are large enough to hold the decoded data.
>>
>> > +
>> > +   mtk_jpeg_set_dec_src(ctx, src_buf, );
>> > +   if (mtk_jpeg_set_dec_dst(ctx, _src_buf->dec_param, dst_buf, 
>> > ))
>> > +   goto dec_end;
>> > +
>> > +   spin_lock_irqsave(>hw_lock, flags);
>> > +   mtk_jpeg_dec_reset(jpeg->dec_reg_base);
>> > +   mtk_jpeg_dec_set_config(jpeg->dec_reg_base,
>> > +   _src_buf->dec_param, , );
>> > +
>> > +   mtk_jpeg_dec_start(jpeg->dec_reg_base);
>> > +   spin_unlock_irqrestore(>hw_lock, flags);
>> > +   return;
>> > +
>> > +dec_end:
>> > +   v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
>> > +   v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>> > +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf), buf_state);
>> > +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(dst_buf), buf_state);
>> > +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>> > +}
>>
>> 
>
>

Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

[Ricky Liang]

Hi Rick,

On Wed, Nov 30, 2016 at 11:08 AM, Rick Chang  wrote:
> Add v4l2 driver for Mediatek JPEG Decoder
>
> Signed-off-by: Rick Chang 
> Signed-off-by: Minghsiu Tsai 



> +static bool mtk_jpeg_check_resolution_change(struct mtk_jpeg_ctx *ctx,
> +struct mtk_jpeg_dec_param *param)
> +{
> +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +   struct mtk_jpeg_q_data *q_data;
> +
> +   q_data = >out_q;
> +   if (q_data->w != param->pic_w || q_data->h != param->pic_h) {
> +   v4l2_dbg(1, debug, >v4l2_dev, "Picture size change\n");
> +   return true;
> +   }
> +
> +   q_data = >cap_q;
> +   if (q_data->fmt != mtk_jpeg_find_format(ctx, param->dst_fourcc,
> +   MTK_JPEG_FMT_TYPE_CAPTURE)) {
> +   v4l2_dbg(1, debug, >v4l2_dev, "format change\n");
> +   return true;
> +   }
> +   return false;



> +static void mtk_jpeg_device_run(void *priv)
> +{
> +   struct mtk_jpeg_ctx *ctx = priv;
> +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +   struct vb2_buffer *src_buf, *dst_buf;
> +   enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +   unsigned long flags;
> +   struct mtk_jpeg_src_buf *jpeg_src_buf;
> +   struct mtk_jpeg_bs bs;
> +   struct mtk_jpeg_fb fb;
> +   int i;
> +
> +   src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> +   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +   jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(src_buf);
> +
> +   if (jpeg_src_buf->flags & MTK_JPEG_BUF_FLAGS_LAST_FRAME) {
> +   for (i = 0; i < dst_buf->num_planes; i++)
> +   vb2_set_plane_payload(dst_buf, i, 0);
> +   buf_state = VB2_BUF_STATE_DONE;
> +   goto dec_end;
> +   }
> +
> +   if (mtk_jpeg_check_resolution_change(ctx, _src_buf->dec_param)) {
> +   mtk_jpeg_queue_src_chg_event(ctx);
> +   ctx->state = MTK_JPEG_SOURCE_CHANGE;
> +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +   return;
> +   }

This only detects source change if multiple OUPUT buffers are queued.
It does not catch the source change in the following scenario:

- OUPUT buffers for jpeg1 enqueued
- OUTPUT queue STREAMON
- userspace creates CAPTURE buffers
- CAPTURE buffers enqueued
- CAPTURE queue STREAMON
- decode
- OUTPUT queue STREAMOFF
- userspace recreates OUTPUT buffers for jpeg2
- OUTPUT buffers for jpeg2 enqueued
- OUTPUT queue STREAMON

In the above sequence if jpeg2's decoded size is larger than jpeg1 the
function fails to detect that the existing CAPTURE buffers are not big
enough to hold the decoded data.

A possible fix is to pass *dst_buf to
mtk_jpeg_check_resolution_change(), and check in the function that all
the dst_buf planes are large enough to hold the decoded data.

> +
> +   mtk_jpeg_set_dec_src(ctx, src_buf, );
> +   if (mtk_jpeg_set_dec_dst(ctx, _src_buf->dec_param, dst_buf, ))
> +   goto dec_end;
> +
> +   spin_lock_irqsave(>hw_lock, flags);
> +   mtk_jpeg_dec_reset(jpeg->dec_reg_base);
> +   mtk_jpeg_dec_set_config(jpeg->dec_reg_base,
> +   _src_buf->dec_param, , );
> +
> +   mtk_jpeg_dec_start(jpeg->dec_reg_base);
> +   spin_unlock_irqrestore(>hw_lock, flags);
> +   return;
> +
> +dec_end:
> +   v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +   v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf), buf_state);
> +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(dst_buf), buf_state);
> +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +}

Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

[Ricky Liang]

Hi Rick,

On Wed, Nov 30, 2016 at 11:08 AM, Rick Chang  wrote:
> Add v4l2 driver for Mediatek JPEG Decoder
>
> Signed-off-by: Rick Chang 
> Signed-off-by: Minghsiu Tsai 



> +static bool mtk_jpeg_check_resolution_change(struct mtk_jpeg_ctx *ctx,
> +struct mtk_jpeg_dec_param *param)
> +{
> +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +   struct mtk_jpeg_q_data *q_data;
> +
> +   q_data = >out_q;
> +   if (q_data->w != param->pic_w || q_data->h != param->pic_h) {
> +   v4l2_dbg(1, debug, >v4l2_dev, "Picture size change\n");
> +   return true;
> +   }
> +
> +   q_data = >cap_q;
> +   if (q_data->fmt != mtk_jpeg_find_format(ctx, param->dst_fourcc,
> +   MTK_JPEG_FMT_TYPE_CAPTURE)) {
> +   v4l2_dbg(1, debug, >v4l2_dev, "format change\n");
> +   return true;
> +   }
> +   return false;



> +static void mtk_jpeg_device_run(void *priv)
> +{
> +   struct mtk_jpeg_ctx *ctx = priv;
> +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +   struct vb2_buffer *src_buf, *dst_buf;
> +   enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +   unsigned long flags;
> +   struct mtk_jpeg_src_buf *jpeg_src_buf;
> +   struct mtk_jpeg_bs bs;
> +   struct mtk_jpeg_fb fb;
> +   int i;
> +
> +   src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> +   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +   jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(src_buf);
> +
> +   if (jpeg_src_buf->flags & MTK_JPEG_BUF_FLAGS_LAST_FRAME) {
> +   for (i = 0; i < dst_buf->num_planes; i++)
> +   vb2_set_plane_payload(dst_buf, i, 0);
> +   buf_state = VB2_BUF_STATE_DONE;
> +   goto dec_end;
> +   }
> +
> +   if (mtk_jpeg_check_resolution_change(ctx, _src_buf->dec_param)) {
> +   mtk_jpeg_queue_src_chg_event(ctx);
> +   ctx->state = MTK_JPEG_SOURCE_CHANGE;
> +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +   return;
> +   }

This only detects source change if multiple OUPUT buffers are queued.
It does not catch the source change in the following scenario:

- OUPUT buffers for jpeg1 enqueued
- OUTPUT queue STREAMON
- userspace creates CAPTURE buffers
- CAPTURE buffers enqueued
- CAPTURE queue STREAMON
- decode
- OUTPUT queue STREAMOFF
- userspace recreates OUTPUT buffers for jpeg2
- OUTPUT buffers for jpeg2 enqueued
- OUTPUT queue STREAMON

In the above sequence if jpeg2's decoded size is larger than jpeg1 the
function fails to detect that the existing CAPTURE buffers are not big
enough to hold the decoded data.

A possible fix is to pass *dst_buf to
mtk_jpeg_check_resolution_change(), and check in the function that all
the dst_buf planes are large enough to hold the decoded data.

> +
> +   mtk_jpeg_set_dec_src(ctx, src_buf, );
> +   if (mtk_jpeg_set_dec_dst(ctx, _src_buf->dec_param, dst_buf, ))
> +   goto dec_end;
> +
> +   spin_lock_irqsave(>hw_lock, flags);
> +   mtk_jpeg_dec_reset(jpeg->dec_reg_base);
> +   mtk_jpeg_dec_set_config(jpeg->dec_reg_base,
> +   _src_buf->dec_param, , );
> +
> +   mtk_jpeg_dec_start(jpeg->dec_reg_base);
> +   spin_unlock_irqrestore(>hw_lock, flags);
> +   return;
> +
> +dec_end:
> +   v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +   v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf), buf_state);
> +   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(dst_buf), buf_state);
> +   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +}

[PATCH] mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels()

[Ricky Liang]

kmemleak reports memory leak in mwifiex_save_hidden_ssid_channels():

unreferenced object 0xffc0a2914780 (size 192):
  comm "ksdioirqd/mmc2", pid 2004, jiffies 4307182506 (age 820.684s)
  hex dump (first 32 bytes):
00 06 47 49 4e 2d 32 67 01 03 c8 60 6c 03 01 40  ..GIN-2g...`l..@
07 10 54 57 20 34 04 1e 64 05 24 84 03 24 95 04  ..TW 4..d.$..$..
  backtrace:
[] create_object+0x164/0x2b4
[] kmemleak_alloc+0x50/0x88
[] __kmalloc_track_caller+0x1bc/0x264
[] kmemdup+0x38/0x64
[] mwifiex_fill_new_bss_desc+0x3c/0x130 [mwifiex]
[] mwifiex_save_curr_bcn+0x4ec/0x640 [mwifiex]
[] mwifiex_handle_event_ext_scan_report+0x1d4/0x268 
[mwifiex]
[] mwifiex_process_sta_event+0x378/0x898 [mwifiex]
[] mwifiex_process_event+0x1a8/0x1e8 [mwifiex]
[] mwifiex_main_process+0x258/0x534 [mwifiex]
[] 0xffbffc258858
[] process_sdio_pending_irqs+0xf8/0x160
[] sdio_irq_thread+0x9c/0x1a4
[] kthread+0xf4/0x100
[] ret_from_fork+0xc/0x50
[] 0x

Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/scan.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
b/drivers/net/wireless/marvell/mwifiex/scan.c
index 97c9765..98ce072 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1671,6 +1671,10 @@ static int mwifiex_save_hidden_ssid_channels(struct 
mwifiex_private *priv,
}
 
 done:
+   /* beacon_ie buffer was allocated in function
+* mwifiex_fill_new_bss_desc(). Free it now.
+*/
+   kfree(bss_desc->beacon_buf);
kfree(bss_desc);
return 0;
 }
-- 
2.6.6

[PATCH] mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels()

[Ricky Liang]

kmemleak reports memory leak in mwifiex_save_hidden_ssid_channels():

unreferenced object 0xffc0a2914780 (size 192):
  comm "ksdioirqd/mmc2", pid 2004, jiffies 4307182506 (age 820.684s)
  hex dump (first 32 bytes):
00 06 47 49 4e 2d 32 67 01 03 c8 60 6c 03 01 40  ..GIN-2g...`l..@
07 10 54 57 20 34 04 1e 64 05 24 84 03 24 95 04  ..TW 4..d.$..$..
  backtrace:
[] create_object+0x164/0x2b4
[] kmemleak_alloc+0x50/0x88
[] __kmalloc_track_caller+0x1bc/0x264
[] kmemdup+0x38/0x64
[] mwifiex_fill_new_bss_desc+0x3c/0x130 [mwifiex]
[] mwifiex_save_curr_bcn+0x4ec/0x640 [mwifiex]
[] mwifiex_handle_event_ext_scan_report+0x1d4/0x268 
[mwifiex]
[] mwifiex_process_sta_event+0x378/0x898 [mwifiex]
[] mwifiex_process_event+0x1a8/0x1e8 [mwifiex]
[] mwifiex_main_process+0x258/0x534 [mwifiex]
[] 0xffbffc258858
[] process_sdio_pending_irqs+0xf8/0x160
[] sdio_irq_thread+0x9c/0x1a4
[] kthread+0xf4/0x100
[] ret_from_fork+0xc/0x50
[] 0x

Signed-off-by: Ricky Liang 
---
 drivers/net/wireless/marvell/mwifiex/scan.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
b/drivers/net/wireless/marvell/mwifiex/scan.c
index 97c9765..98ce072 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1671,6 +1671,10 @@ static int mwifiex_save_hidden_ssid_channels(struct 
mwifiex_private *priv,
}
 
 done:
+   /* beacon_ie buffer was allocated in function
+* mwifiex_fill_new_bss_desc(). Free it now.
+*/
+   kfree(bss_desc->beacon_buf);
kfree(bss_desc);
return 0;
 }
-- 
2.6.6

[PATCH v3] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

This patch fixes the issue by allocating a buffer which is big enough for
SDIO_BLOCK_SIZE transfer and/or BTSDIO_DMA_ALIGN address relocation.

CC: Wei-Ning Huang <wnhu...@chromium.org>
CC: Daniel Kurtz <djku...@chromium.org>
CC: Amitkumar Karwar <akar...@marvell.com>

Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 drivers/bluetooth/btmrvl_sdio.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..b7c3928 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,7 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
int blksz;
int i = 0;
u8 *buf = NULL;
@@ -1083,9 +1082,13 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
return -EINVAL;
}
 
+   blksz = DIV_ROUND_UP(nb, SDIO_BLOCK_SIZE) * SDIO_BLOCK_SIZE;
+
buf = payload;
-   if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1) ||
+   nb < blksz) {
+   tmpbufsz = ALIGN_SZ(blksz, BTSDIO_DMA_ALIGN) +
+  BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1096,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  blksz);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

[PATCH v3] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

This patch fixes the issue by allocating a buffer which is big enough for
SDIO_BLOCK_SIZE transfer and/or BTSDIO_DMA_ALIGN address relocation.

CC: Wei-Ning Huang 
CC: Daniel Kurtz 
CC: Amitkumar Karwar 

Signed-off-by: Ricky Liang 
---
 drivers/bluetooth/btmrvl_sdio.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..b7c3928 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,7 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
int blksz;
int i = 0;
u8 *buf = NULL;
@@ -1083,9 +1082,13 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
return -EINVAL;
}
 
+   blksz = DIV_ROUND_UP(nb, SDIO_BLOCK_SIZE) * SDIO_BLOCK_SIZE;
+
buf = payload;
-   if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1) ||
+   nb < blksz) {
+   tmpbufsz = ALIGN_SZ(blksz, BTSDIO_DMA_ALIGN) +
+  BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1096,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  blksz);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

[PATCH v2] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

Since sdio_writesb() is able to handle any size of data it's not necessary
to re-allocate memroy region which size is multiple of SDIO_BLOCK_SIZE. We
just need to make sure the relocation of memory for making the address
BTSDIO_DMA_ALIGN-aligned does not cause out-of-bound access.

CC: Wei-Ning Huang <wnhu...@chromium.org>
CC: Daniel Kurtz <djku...@chromium.org>
CC: Amitkumar Karwar <akar...@marvell.com>

Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 drivers/bluetooth/btmrvl_sdio.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..ecc0191 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,8 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
-   int blksz;
int i = 0;
u8 *buf = NULL;
void *tmpbuf = NULL;
@@ -1085,7 +1083,7 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 
buf = payload;
if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN) + BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1091,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  nb);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

[PATCH v2] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

Since sdio_writesb() is able to handle any size of data it's not necessary
to re-allocate memroy region which size is multiple of SDIO_BLOCK_SIZE. We
just need to make sure the relocation of memory for making the address
BTSDIO_DMA_ALIGN-aligned does not cause out-of-bound access.

CC: Wei-Ning Huang 
CC: Daniel Kurtz 
CC: Amitkumar Karwar 

Signed-off-by: Ricky Liang 
---
 drivers/bluetooth/btmrvl_sdio.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..ecc0191 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,8 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
-   int blksz;
int i = 0;
u8 *buf = NULL;
void *tmpbuf = NULL;
@@ -1085,7 +1083,7 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 
buf = payload;
if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN) + BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1091,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  nb);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

[PATCH] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

Since sdio_writesb() is able to handle any size of data it's not necessary
to re-allocate memroy region which size is multiple of SDIO_BLOCK_SIZE. We
just need to make sure the relocation of memory for making the address
BTSDIO_DMA_ALIGN-aligned does not cause out-of-bound access.

CC: Wei-Ning Huang <wnhu...@chromium.org>
CC: Daniel Kurtz <djku...@chromium.org>
CC: Amitkumar Karwar <akar...@marvell.com>

Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 drivers/bluetooth/btmrvl_sdio.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..ecc0191 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,8 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
-   int blksz;
int i = 0;
u8 *buf = NULL;
void *tmpbuf = NULL;
@@ -1085,7 +1083,7 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 
buf = payload;
if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN) + BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1091,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  nb);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

[PATCH] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

Since sdio_writesb() is able to handle any size of data it's not necessary
to re-allocate memroy region which size is multiple of SDIO_BLOCK_SIZE. We
just need to make sure the relocation of memory for making the address
BTSDIO_DMA_ALIGN-aligned does not cause out-of-bound access.

CC: Wei-Ning Huang 
CC: Daniel Kurtz 
CC: Amitkumar Karwar 

Signed-off-by: Ricky Liang 
---
 drivers/bluetooth/btmrvl_sdio.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..ecc0191 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,8 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
-   int blksz;
int i = 0;
u8 *buf = NULL;
void *tmpbuf = NULL;
@@ -1085,7 +1083,7 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 
buf = payload;
if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN) + BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1091,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  nb);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

[PATCH] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

This patch fixes the issue by allocating a buffer which is big enough for
SDIO_BLOCK_SIZE transfer and/or BTSDIO_DMA_ALIGN address relocation.

Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 drivers/bluetooth/btmrvl_sdio.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..0bfeb19 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,7 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
int blksz;
int i = 0;
u8 *buf = NULL;
@@ -1083,9 +1082,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
return -EINVAL;
}
 
+   blksz = DIV_ROUND_UP(nb, SDIO_BLOCK_SIZE) * SDIO_BLOCK_SIZE;
+
buf = payload;
if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   tmpbufsz = ALIGN_SZ(blksz, BTSDIO_DMA_ALIGN) +
+  BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1095,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  blksz);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

[PATCH] Bluetooth: btmrvl: fix slab-out-of-bounds access in btmrvl_sdio

[Ricky Liang]

Kasan reported slab-out-of-bounds access in btmrvl_sdio:

[   33.055400] 
==
[   33.062585] BUG: KASAN: slab-out-of-bounds in memcpy+0x24/0x50 at addr 
ffc0d89b4a00
[   33.070529] Read of size 256 by task btmrvl_main_ser/3576
[   33.075885] 
=
[   33.084002] BUG kmalloc-256 (Tainted: GB ): kasan: bad access 
detected
[   33.091511] 
-

[   33.413498] Call trace:
[   33.415928] [] dump_backtrace+0x0/0x190
[   33.421288] [] show_stack+0x1c/0x28
[   33.426305] [] dump_stack+0xa0/0xf8
[   33.431320] [] print_trailer+0x158/0x16c
[   33.436765] [] object_err+0x48/0x5c
[   33.441780] [] kasan_report+0x344/0x510
[   33.447141] [] __asan_loadN+0x20/0x150
[   33.452413] [] memcpy+0x20/0x50
[   33.457084] [] swiotlb_tbl_map_single+0x2ec/0x310
[   33.463305] [] map_single+0x24/0x30
[   33.468320] [] swiotlb_map_sg_attrs+0xec/0x21c
[   33.474286] [] __swiotlb_map_sg_attrs+0x48/0xec
[   33.480339] [] msdc_prepare_data.isra.11+0xf0/0x11c
[   33.486733] [] msdc_ops_request+0x74/0xf0
[   33.492266] [] __mmc_start_request+0x78/0x8c
[   33.498057] [] mmc_start_request+0x220/0x240
[   33.503848] [] mmc_wait_for_req+0x78/0x250
[   33.509468] [] mmc_io_rw_extended+0x2ec/0x388
[   33.515347] [] sdio_io_rw_ext_helper+0x160/0x268
[   33.521483] [] sdio_writesb+0x40/0x50
[   33.526677] [] btmrvl_sdio_host_to_card+0x124/0x1bc 
[btmrvl_sdio]
[   33.534283] [] btmrvl_service_main_thread+0x384/0x428 
[btmrvl]
[   33.541626] [] kthread+0x140/0x158
[   33.546550] Memory state around the buggy address:
[   33.551305]  ffc0d89b4980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.558474]  ffc0d89b4a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[   33.565643] >ffc0d89b4a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
fc
[   33.572809] ^
[   33.579889]  ffc0d89b4b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.587055]  ffc0d89b4b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[   33.594221] 
==

The cause of this is that btmrvl_sdio_host_to_card can access memory region
out of its allocated space due to:

  1. the requested block size is smaller than SDIO_BLOCK_SIZE, and/or
  2. the allocated memory is not BTSDIO_DMA_ALIGN-aligned.

This patch fixes the issue by allocating a buffer which is big enough for
SDIO_BLOCK_SIZE transfer and/or BTSDIO_DMA_ALIGN address relocation.

Signed-off-by: Ricky Liang 
---
 drivers/bluetooth/btmrvl_sdio.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index f425ddf..0bfeb19 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1071,7 +1071,6 @@ static int btmrvl_sdio_host_to_card(struct btmrvl_private 
*priv,
 {
struct btmrvl_sdio_card *card = priv->btmrvl_dev.card;
int ret = 0;
-   int buf_block_len;
int blksz;
int i = 0;
u8 *buf = NULL;
@@ -1083,9 +1082,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
return -EINVAL;
}
 
+   blksz = DIV_ROUND_UP(nb, SDIO_BLOCK_SIZE) * SDIO_BLOCK_SIZE;
+
buf = payload;
if ((unsigned long) payload & (BTSDIO_DMA_ALIGN - 1)) {
-   tmpbufsz = ALIGN_SZ(nb, BTSDIO_DMA_ALIGN);
+   tmpbufsz = ALIGN_SZ(blksz, BTSDIO_DMA_ALIGN) +
+  BTSDIO_DMA_ALIGN;
tmpbuf = kzalloc(tmpbufsz, GFP_KERNEL);
if (!tmpbuf)
return -ENOMEM;
@@ -1093,15 +1095,12 @@ static int btmrvl_sdio_host_to_card(struct 
btmrvl_private *priv,
memcpy(buf, payload, nb);
}
 
-   blksz = SDIO_BLOCK_SIZE;
-   buf_block_len = DIV_ROUND_UP(nb, blksz);
-
sdio_claim_host(card->func);
 
do {
/* Transfer data to card */
ret = sdio_writesb(card->func, card->ioport, buf,
-  buf_block_len * blksz);
+  blksz);
if (ret < 0) {
i++;
BT_ERR("i=%d writesb failed: %d", i, ret);
-- 
2.1.2

Re: [PATCH] Input: uinput - handle compat ioctl for UI_SET_PHYS

[Ricky Liang]

Hi Dmitry,

On Sat, May 21, 2016 at 12:32 AM, Dmitry Torokhov
<dmitry.torok...@gmail.com> wrote:
> Hi Ricky,
>
> On Tue, May 17, 2016 at 11:39:45PM +0800, Ricky Liang wrote:
>> When running a 32-bit userspace on a 64-bit kernel, the UI_SET_PHYS
>> ioctl needs to be treated with special care, as it has the pointer
>> size encoded in the command.
>>
>> Signed-off-by: Ricky Liang <jcli...@chromium.org>
>> ---
>>  drivers/input/misc/uinput.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index abe1a92..b4d1b1d 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -984,6 +984,15 @@ static long uinput_ioctl(struct file *file, unsigned 
>> int cmd, unsigned long arg)
>>  static long uinput_compat_ioctl(struct file *file,
>>   unsigned int cmd, unsigned long arg)
>>  {
>> + switch (_IOC_NR(cmd)) {
>> + case _IOC_NR(UI_SET_PHYS):
>> + if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
>> + cmd &= ~IOCSIZE_MASK;
>> + cmd |= sizeof(void *) << IOCSIZE_SHIFT;
>> + }
>> + break;
>> + }
>> +
>
> This looks quite complicated... Can we do this:
>
> #define UI_SET_PHYS_COMPAT __IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
>
> ...
>
> if (cmd == UI_SET_PHYS_COMPAT)
> cmd = UI_SET_PHYS;
>
>>   return uinput_ioctl_handler(file, cmd, arg, compat_ptr(arg));
>>  }
>>  #endif
>
> We can use the local define instead of manipulating cmd size because we
> will never going to change UI_SET_PHYS definition, since it is part of
> uapi.

Sounds good. I'll send v2 to implement this.

Thanks,
Ricky

>
> Thanks.
>
> --
> Dmitry

Re: [PATCH] Input: uinput - handle compat ioctl for UI_SET_PHYS

[Ricky Liang]

Hi Dmitry,

On Sat, May 21, 2016 at 12:32 AM, Dmitry Torokhov
 wrote:
> Hi Ricky,
>
> On Tue, May 17, 2016 at 11:39:45PM +0800, Ricky Liang wrote:
>> When running a 32-bit userspace on a 64-bit kernel, the UI_SET_PHYS
>> ioctl needs to be treated with special care, as it has the pointer
>> size encoded in the command.
>>
>> Signed-off-by: Ricky Liang 
>> ---
>>  drivers/input/misc/uinput.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index abe1a92..b4d1b1d 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -984,6 +984,15 @@ static long uinput_ioctl(struct file *file, unsigned 
>> int cmd, unsigned long arg)
>>  static long uinput_compat_ioctl(struct file *file,
>>   unsigned int cmd, unsigned long arg)
>>  {
>> + switch (_IOC_NR(cmd)) {
>> + case _IOC_NR(UI_SET_PHYS):
>> + if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
>> + cmd &= ~IOCSIZE_MASK;
>> + cmd |= sizeof(void *) << IOCSIZE_SHIFT;
>> + }
>> + break;
>> + }
>> +
>
> This looks quite complicated... Can we do this:
>
> #define UI_SET_PHYS_COMPAT __IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
>
> ...
>
> if (cmd == UI_SET_PHYS_COMPAT)
> cmd = UI_SET_PHYS;
>
>>   return uinput_ioctl_handler(file, cmd, arg, compat_ptr(arg));
>>  }
>>  #endif
>
> We can use the local define instead of manipulating cmd size because we
> will never going to change UI_SET_PHYS definition, since it is part of
> uapi.

Sounds good. I'll send v2 to implement this.

Thanks,
Ricky

>
> Thanks.
>
> --
> Dmitry

[PATCH v2] Input: uinput - handle compat ioctl for UI_SET_PHYS

[Ricky Liang]

When running a 32-bit userspace on a 64-bit kernel, the UI_SET_PHYS
ioctl needs to be treated with special care, as it has the pointer
size encoded in the command.

Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 drivers/input/misc/uinput.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index abe1a92..65ebbd1 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -981,9 +981,15 @@ static long uinput_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
 }
 
 #ifdef CONFIG_COMPAT
+
+#define UI_SET_PHYS_COMPAT _IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
+
 static long uinput_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
 {
+   if (cmd == UI_SET_PHYS_COMPAT)
+   cmd = UI_SET_PHYS;
+
return uinput_ioctl_handler(file, cmd, arg, compat_ptr(arg));
 }
 #endif
-- 
2.1.2

[PATCH v2] Input: uinput - handle compat ioctl for UI_SET_PHYS

[Ricky Liang]

When running a 32-bit userspace on a 64-bit kernel, the UI_SET_PHYS
ioctl needs to be treated with special care, as it has the pointer
size encoded in the command.

Signed-off-by: Ricky Liang 
---
 drivers/input/misc/uinput.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index abe1a92..65ebbd1 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -981,9 +981,15 @@ static long uinput_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
 }
 
 #ifdef CONFIG_COMPAT
+
+#define UI_SET_PHYS_COMPAT _IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
+
 static long uinput_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
 {
+   if (cmd == UI_SET_PHYS_COMPAT)
+   cmd = UI_SET_PHYS;
+
return uinput_ioctl_handler(file, cmd, arg, compat_ptr(arg));
 }
 #endif
-- 
2.1.2

[PATCH] Input: uinput - handle compat ioctl for UI_SET_PHYS

[Ricky Liang]

When running a 32-bit userspace on a 64-bit kernel, the UI_SET_PHYS
ioctl needs to be treated with special care, as it has the pointer
size encoded in the command.

Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 drivers/input/misc/uinput.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index abe1a92..b4d1b1d 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -984,6 +984,15 @@ static long uinput_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
 static long uinput_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
 {
+   switch (_IOC_NR(cmd)) {
+   case _IOC_NR(UI_SET_PHYS):
+   if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
+   cmd &= ~IOCSIZE_MASK;
+   cmd |= sizeof(void *) << IOCSIZE_SHIFT;
+   }
+   break;
+   }
+
return uinput_ioctl_handler(file, cmd, arg, compat_ptr(arg));
 }
 #endif
-- 
2.1.2

[PATCH] Input: uinput - handle compat ioctl for UI_SET_PHYS

[Ricky Liang]

When running a 32-bit userspace on a 64-bit kernel, the UI_SET_PHYS
ioctl needs to be treated with special care, as it has the pointer
size encoded in the command.

Signed-off-by: Ricky Liang 
---
 drivers/input/misc/uinput.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index abe1a92..b4d1b1d 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -984,6 +984,15 @@ static long uinput_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
 static long uinput_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
 {
+   switch (_IOC_NR(cmd)) {
+   case _IOC_NR(UI_SET_PHYS):
+   if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
+   cmd &= ~IOCSIZE_MASK;
+   cmd |= sizeof(void *) << IOCSIZE_SHIFT;
+   }
+   break;
+   }
+
return uinput_ioctl_handler(file, cmd, arg, compat_ptr(arg));
 }
 #endif
-- 
2.1.2

Re: [PATCH] thermal: fix thermal_power_allocator trace event

[Ricky Liang]

Please ignore this patch. I found out that I missed out the patch that
changed the second parameter of __print_array() to number of elements.

On Tue, Mar 29, 2016 at 1:08 PM, Ricky Liang <jcli...@chromium.org> wrote:
> Fix the dynamic array length in printing the thermal_power_allocator
> trace event.
>
> CC: Javi Merino <javi.mer...@arm.com>
> CC: Daniel Kurtz <djku...@chromium.org>
> Signed-off-by: Ricky Liang <jcli...@chromium.org>
> ---
>  include/trace/events/thermal_power_allocator.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/trace/events/thermal_power_allocator.h 
> b/include/trace/events/thermal_power_allocator.h
> index 5afae8f..85e5391 100644
> --- a/include/trace/events/thermal_power_allocator.h
> +++ b/include/trace/events/thermal_power_allocator.h
> @@ -45,10 +45,10 @@ TRACE_EVENT(thermal_power_allocator,
> TP_printk("thermal_zone_id=%d req_power={%s} total_req_power=%u 
> granted_power={%s} total_granted_power=%u power_range=%u 
> max_allocatable_power=%u current_temperature=%d delta_temperature=%d",
> __entry->tz_id,
> __print_array(__get_dynamic_array(req_power),
> -  __entry->num_actors, 4),
> +  __get_dynamic_array_len(req_power), 4),
> __entry->total_req_power,
> __print_array(__get_dynamic_array(granted_power),
> -  __entry->num_actors, 4),
> +  __get_dynamic_array_len(granted_power), 4),
> __entry->total_granted_power, __entry->power_range,
> __entry->max_allocatable_power, __entry->current_temp,
> __entry->delta_temp)
> --
> 2.1.2
>

Re: [PATCH] thermal: fix thermal_power_allocator trace event

[Ricky Liang]

Please ignore this patch. I found out that I missed out the patch that
changed the second parameter of __print_array() to number of elements.

On Tue, Mar 29, 2016 at 1:08 PM, Ricky Liang  wrote:
> Fix the dynamic array length in printing the thermal_power_allocator
> trace event.
>
> CC: Javi Merino 
> CC: Daniel Kurtz 
> Signed-off-by: Ricky Liang 
> ---
>  include/trace/events/thermal_power_allocator.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/trace/events/thermal_power_allocator.h 
> b/include/trace/events/thermal_power_allocator.h
> index 5afae8f..85e5391 100644
> --- a/include/trace/events/thermal_power_allocator.h
> +++ b/include/trace/events/thermal_power_allocator.h
> @@ -45,10 +45,10 @@ TRACE_EVENT(thermal_power_allocator,
> TP_printk("thermal_zone_id=%d req_power={%s} total_req_power=%u 
> granted_power={%s} total_granted_power=%u power_range=%u 
> max_allocatable_power=%u current_temperature=%d delta_temperature=%d",
> __entry->tz_id,
> __print_array(__get_dynamic_array(req_power),
> -  __entry->num_actors, 4),
> +  __get_dynamic_array_len(req_power), 4),
> __entry->total_req_power,
> __print_array(__get_dynamic_array(granted_power),
> -  __entry->num_actors, 4),
> +  __get_dynamic_array_len(granted_power), 4),
> __entry->total_granted_power, __entry->power_range,
> __entry->max_allocatable_power, __entry->current_temp,
> __entry->delta_temp)
> --
> 2.1.2
>

[PATCH] thermal: fix thermal_power_allocator trace event

[Ricky Liang]

Fix the dynamic array length in printing the thermal_power_allocator
trace event.

CC: Javi Merino <javi.mer...@arm.com>
CC: Daniel Kurtz <djku...@chromium.org>
Signed-off-by: Ricky Liang <jcli...@chromium.org>
---
 include/trace/events/thermal_power_allocator.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/thermal_power_allocator.h 
b/include/trace/events/thermal_power_allocator.h
index 5afae8f..85e5391 100644
--- a/include/trace/events/thermal_power_allocator.h
+++ b/include/trace/events/thermal_power_allocator.h
@@ -45,10 +45,10 @@ TRACE_EVENT(thermal_power_allocator,
TP_printk("thermal_zone_id=%d req_power={%s} total_req_power=%u 
granted_power={%s} total_granted_power=%u power_range=%u 
max_allocatable_power=%u current_temperature=%d delta_temperature=%d",
__entry->tz_id,
__print_array(__get_dynamic_array(req_power),
-  __entry->num_actors, 4),
+  __get_dynamic_array_len(req_power), 4),
__entry->total_req_power,
__print_array(__get_dynamic_array(granted_power),
-  __entry->num_actors, 4),
+  __get_dynamic_array_len(granted_power), 4),
__entry->total_granted_power, __entry->power_range,
__entry->max_allocatable_power, __entry->current_temp,
__entry->delta_temp)
-- 
2.1.2

[PATCH] thermal: fix thermal_power_allocator trace event

[Ricky Liang]

Fix the dynamic array length in printing the thermal_power_allocator
trace event.

CC: Javi Merino 
CC: Daniel Kurtz 
Signed-off-by: Ricky Liang 
---
 include/trace/events/thermal_power_allocator.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/thermal_power_allocator.h 
b/include/trace/events/thermal_power_allocator.h
index 5afae8f..85e5391 100644
--- a/include/trace/events/thermal_power_allocator.h
+++ b/include/trace/events/thermal_power_allocator.h
@@ -45,10 +45,10 @@ TRACE_EVENT(thermal_power_allocator,
TP_printk("thermal_zone_id=%d req_power={%s} total_req_power=%u 
granted_power={%s} total_granted_power=%u power_range=%u 
max_allocatable_power=%u current_temperature=%d delta_temperature=%d",
__entry->tz_id,
__print_array(__get_dynamic_array(req_power),
-  __entry->num_actors, 4),
+  __get_dynamic_array_len(req_power), 4),
__entry->total_req_power,
__print_array(__get_dynamic_array(granted_power),
-  __entry->num_actors, 4),
+  __get_dynamic_array_len(granted_power), 4),
__entry->total_granted_power, __entry->power_range,
__entry->max_allocatable_power, __entry->current_temp,
__entry->delta_temp)
-- 
2.1.2

Re: [RFCv7 PATCH 04/10] sched/fair: add triggers for OPP change requests

[Ricky Liang]

Hi Steve,

On Tue, Feb 23, 2016 at 9:22 AM, Steve Muckle  wrote:
> From: Juri Lelli 
>
> Each time a task is {en,de}queued we might need to adapt the current
> frequency to the new usage. Add triggers on {en,de}queue_task_fair() for
> this purpose.  Only trigger a freq request if we are effectively waking up
> or going to sleep.  Filter out load balancing related calls to reduce the
> number of triggers.
>
> [smuc...@linaro.org: resolve merge conflicts, define task_new,
>  use renamed static key sched_freq]
>
> cc: Ingo Molnar 
> cc: Peter Zijlstra 
> Signed-off-by: Juri Lelli 
> Signed-off-by: Steve Muckle 
> ---
>  kernel/sched/fair.c | 49 +++--
>  1 file changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3437e01..f1f00a4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4283,6 +4283,21 @@ static inline void hrtick_update(struct rq *rq)
>  }
>  #endif
>
> +static unsigned long capacity_orig_of(int cpu);
> +static int cpu_util(int cpu);
> +
> +static void update_capacity_of(int cpu)
> +{
> +   unsigned long req_cap;
> +
> +   if (!sched_freq())
> +   return;
> +
> +   /* Convert scale-invariant capacity to cpu. */
> +   req_cap = cpu_util(cpu) * SCHED_CAPACITY_SCALE / 
> capacity_orig_of(cpu);
> +   set_cfs_cpu_capacity(cpu, true, req_cap);
> +}
> +

The change hunks of this patch should probably all depend on
CONFIG_SMP as capacity_orig_of() and cpu_util() are only available
when CONFIG_SMP is enabled.

[snip...]

Thanks,
Ricky

Re: [RFCv7 PATCH 04/10] sched/fair: add triggers for OPP change requests

[Ricky Liang]

Hi Steve,

On Tue, Feb 23, 2016 at 9:22 AM, Steve Muckle  wrote:
> From: Juri Lelli 
>
> Each time a task is {en,de}queued we might need to adapt the current
> frequency to the new usage. Add triggers on {en,de}queue_task_fair() for
> this purpose.  Only trigger a freq request if we are effectively waking up
> or going to sleep.  Filter out load balancing related calls to reduce the
> number of triggers.
>
> [smuc...@linaro.org: resolve merge conflicts, define task_new,
>  use renamed static key sched_freq]
>
> cc: Ingo Molnar 
> cc: Peter Zijlstra 
> Signed-off-by: Juri Lelli 
> Signed-off-by: Steve Muckle 
> ---
>  kernel/sched/fair.c | 49 +++--
>  1 file changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3437e01..f1f00a4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4283,6 +4283,21 @@ static inline void hrtick_update(struct rq *rq)
>  }
>  #endif
>
> +static unsigned long capacity_orig_of(int cpu);
> +static int cpu_util(int cpu);
> +
> +static void update_capacity_of(int cpu)
> +{
> +   unsigned long req_cap;
> +
> +   if (!sched_freq())
> +   return;
> +
> +   /* Convert scale-invariant capacity to cpu. */
> +   req_cap = cpu_util(cpu) * SCHED_CAPACITY_SCALE / 
> capacity_orig_of(cpu);
> +   set_cfs_cpu_capacity(cpu, true, req_cap);
> +}
> +

The change hunks of this patch should probably all depend on
CONFIG_SMP as capacity_orig_of() and cpu_util() are only available
when CONFIG_SMP is enabled.

[snip...]

Thanks,
Ricky

Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection

[Ricky Liang]

Hi Steve,

On Wed, Dec 9, 2015 at 2:19 PM, Steve Muckle  wrote:

[snip...]

> +static int cpufreq_sched_policy_init(struct cpufreq_policy *policy)
> +{
> +   struct gov_data *gd;
> +   int cpu;
> +
> +   for_each_cpu(cpu, policy->cpus)
> +   memset(_cpu(cpu_sched_capacity_reqs, cpu), 0,
> +  sizeof(struct sched_capacity_reqs));
> +
> +   gd = kzalloc(sizeof(*gd), GFP_KERNEL);
> +   if (!gd)
> +   return -ENOMEM;
> +
> +   gd->throttle_nsec = policy->cpuinfo.transition_latency ?
> +   policy->cpuinfo.transition_latency :
> +   THROTTLE_NSEC;
> +   pr_debug("%s: throttle threshold = %u [ns]\n",
> + __func__, gd->throttle_nsec);
> +
> +   if (cpufreq_driver_is_slow()) {
> +   cpufreq_driver_slow = true;
> +   gd->task = kthread_create(cpufreq_sched_thread, policy,
> + "kschedfreq:%d",
> + 
> cpumask_first(policy->related_cpus));
> +   if (IS_ERR_OR_NULL(gd->task)) {
> +   pr_err("%s: failed to create kschedfreq thread\n",
> +  __func__);
> +   goto err;
> +   }
> +   get_task_struct(gd->task);
> +   kthread_bind_mask(gd->task, policy->related_cpus);
> +   wake_up_process(gd->task);
> +   init_irq_work(>irq_work, cpufreq_sched_irq_work);
> +   }
> +
> +   policy->governor_data = gd;

This should be moved before if(cpufreq_driver_is_slow()) {...}. I've
seen NULL pointer deference at boot in cpufreq_sched_thread() when it
tried to run sched_setscheduler_nocheck(gd->task, SCHED_FIFO, ).

> +   set_sched_freq();
> +
> +   return 0;
> +
> +err:

And probably also set policy->governor_data to NULL here.

> +   kfree(gd);
> +   return -ENOMEM;
> +}

[snip...]

Best,
Ricky

Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection

[Ricky Liang]

Hi Steve,

On Wed, Dec 9, 2015 at 2:19 PM, Steve Muckle  wrote:

[snip...]

> +static int cpufreq_sched_policy_init(struct cpufreq_policy *policy)
> +{
> +   struct gov_data *gd;
> +   int cpu;
> +
> +   for_each_cpu(cpu, policy->cpus)
> +   memset(_cpu(cpu_sched_capacity_reqs, cpu), 0,
> +  sizeof(struct sched_capacity_reqs));
> +
> +   gd = kzalloc(sizeof(*gd), GFP_KERNEL);
> +   if (!gd)
> +   return -ENOMEM;
> +
> +   gd->throttle_nsec = policy->cpuinfo.transition_latency ?
> +   policy->cpuinfo.transition_latency :
> +   THROTTLE_NSEC;
> +   pr_debug("%s: throttle threshold = %u [ns]\n",
> + __func__, gd->throttle_nsec);
> +
> +   if (cpufreq_driver_is_slow()) {
> +   cpufreq_driver_slow = true;
> +   gd->task = kthread_create(cpufreq_sched_thread, policy,
> + "kschedfreq:%d",
> + 
> cpumask_first(policy->related_cpus));
> +   if (IS_ERR_OR_NULL(gd->task)) {
> +   pr_err("%s: failed to create kschedfreq thread\n",
> +  __func__);
> +   goto err;
> +   }
> +   get_task_struct(gd->task);
> +   kthread_bind_mask(gd->task, policy->related_cpus);
> +   wake_up_process(gd->task);
> +   init_irq_work(>irq_work, cpufreq_sched_irq_work);
> +   }
> +
> +   policy->governor_data = gd;

This should be moved before if(cpufreq_driver_is_slow()) {...}. I've
seen NULL pointer deference at boot in cpufreq_sched_thread() when it
tried to run sched_setscheduler_nocheck(gd->task, SCHED_FIFO, ).

> +   set_sched_freq();
> +
> +   return 0;
> +
> +err:

And probably also set policy->governor_data to NULL here.

> +   kfree(gd);
> +   return -ENOMEM;
> +}

[snip...]

Best,
Ricky

Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection

[Ricky Liang]

Hi Steve,

On Wed, Dec 9, 2015 at 2:19 PM, Steve Muckle  wrote:

[...]

> +/*
> + * we pass in struct cpufreq_policy. This is safe because changing out the
> + * policy requires a call to __cpufreq_governor(policy, CPUFREQ_GOV_STOP),
> + * which tears down all of the data structures and __cpufreq_governor(policy,
> + * CPUFREQ_GOV_START) will do a full rebuild, including this kthread with the
> + * new policy pointer
> + */
> +static int cpufreq_sched_thread(void *data)
> +{
> +   struct sched_param param;
> +   struct cpufreq_policy *policy;
> +   struct gov_data *gd;
> +   unsigned int new_request = 0;
> +   unsigned int last_request = 0;
> +   int ret;
> +
> +   policy = (struct cpufreq_policy *) data;
> +   gd = policy->governor_data;
> +
> +   param.sched_priority = 50;
> +   ret = sched_setscheduler_nocheck(gd->task, SCHED_FIFO, );
> +   if (ret) {
> +   pr_warn("%s: failed to set SCHED_FIFO\n", __func__);
> +   do_exit(-EINVAL);
> +   } else {
> +   pr_debug("%s: kthread (%d) set to SCHED_FIFO\n",
> +   __func__, gd->task->pid);
> +   }
> +
> +   do {
> +   set_current_state(TASK_INTERRUPTIBLE);
> +   new_request = gd->requested_freq;
> +   if (new_request == last_request) {
> +   schedule();

Should we check kthread_should_stop() after
set_current_state(TASK_INTERRUPTIBLE), probably right before
schedule()? Something like:

   set_current_state(TASK_INTERRUPTIBLE);
   new_request = gd->requested_freq;
   if (new_request == last_request) {
   if (kthread_should_stop())
   break;
   schedule();
   } else {
   ...
   }

On the previous version of the scheduler-driver cpu frequency
selection I had the following:

<3>[ 1920.233598] INFO: task autotest:32443 blocked for more than 120 seconds.
<3>[ 1920.233625]   Not tainted 3.18.0-09696-g4312b25 #1
<3>[ 1920.233641] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
<6>[ 1920.233659] autotestD ffc0002057a0 0 32443
32403 0x0040
<0>[ 1920.233693] Call trace:
<4>[ 1920.233724] [] __switch_to+0x80/0x8c
<4>[ 1920.233748] [] __schedule+0x550/0x7d8
<4>[ 1920.233769] [] schedule+0x78/0x84
<4>[ 1920.233786] [] schedule_timeout+0x40/0x2ac
<4>[ 1920.233804] [] wait_for_common+0x154/0x18c
<4>[ 1920.233820] [] wait_for_completion+0x24/0x34
<4>[ 1920.233840] [] kthread_stop+0x130/0x22c
<4>[ 1920.233859] [] cpufreq_sched_setup+0x21c/0x308
<4>[ 1920.233881] [] __cpufreq_governor+0x114/0x1c8
<4>[ 1920.233901] [] cpufreq_set_policy+0x120/0x1b8
<4>[ 1920.233920] [] store_scaling_governor+0x8c/0xd4
<4>[ 1920.233937] [] store+0x98/0xd0
<4>[ 1920.233958] [] sysfs_kf_write+0x54/0x64
<4>[ 1920.233977] [] kernfs_fop_write+0x108/0x150
<4>[ 1920.233999] [] vfs_write+0xc4/0x1a0
<4>[ 1920.234018] [] SyS_write+0x60/0xb4
<4>[ 1920.234031] INFO: lockdep is turned off.
<6>[ 1920.234043]   taskPC stack   pid father
<6>[ 1920.234161] autotestD ffc0002057a0 0 32443
32403 0x0040
<0>[ 1920.234193] Call trace:
<4>[ 1920.234211] [] __switch_to+0x80/0x8c
<4>[ 1920.234232] [] __schedule+0x550/0x7d8
<4>[ 1920.234251] [] schedule+0x78/0x84
<4>[ 1920.234268] [] schedule_timeout+0x40/0x2ac
<4>[ 1920.234285] [] wait_for_common+0x154/0x18c
<4>[ 1920.234301] [] wait_for_completion+0x24/0x34
<4>[ 1920.234319] [] kthread_stop+0x130/0x22c
<4>[ 1920.234335] [] cpufreq_sched_setup+0x21c/0x308
<4>[ 1920.234355] [] __cpufreq_governor+0x114/0x1c8
<4>[ 1920.234375] [] cpufreq_set_policy+0x120/0x1b8
<4>[ 1920.234395] [] store_scaling_governor+0x8c/0xd4
<4>[ 1920.234413] [] store+0x98/0xd0
<4>[ 1920.234432] [] sysfs_kf_write+0x54/0x64
<4>[ 1920.234449] [] kernfs_fop_write+0x108/0x150
<4>[ 1920.234470] [] vfs_write+0xc4/0x1a0
<4>[ 1920.234489] [] SyS_write+0x60/0xb4

This happened while the kernel is switching from the sched governor to
the userspace governor. There's a race between kthread_stop() and
cpufreq_sched_thread(). On the previous version I was testing, I can
easily reproduce the lockup if I add a msleep(100) right before
set_current_state(TASK_INTERRUPTIBLE), and then switching between the
two governors through sysfs.

> +   } else {
> +   /*
> +* if the frequency thread sleeps while waiting to be
> +* unthrottled, start over to check for a newer 
> request
> +*/
> +   if (finish_last_request(gd))
> +   continue;
> +   last_request = new_request;
> +   cpufreq_sched_try_driver_target(policy, new_request);
> +   }
> +   } while (!kthread_should_stop());
> +
> +   return 0;
> +}

[...]

Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection

[Ricky Liang]

Hi Steve,

On Wed, Dec 9, 2015 at 2:19 PM, Steve Muckle  wrote:

[...]

> +/*
> + * we pass in struct cpufreq_policy. This is safe because changing out the
> + * policy requires a call to __cpufreq_governor(policy, CPUFREQ_GOV_STOP),
> + * which tears down all of the data structures and __cpufreq_governor(policy,
> + * CPUFREQ_GOV_START) will do a full rebuild, including this kthread with the
> + * new policy pointer
> + */
> +static int cpufreq_sched_thread(void *data)
> +{
> +   struct sched_param param;
> +   struct cpufreq_policy *policy;
> +   struct gov_data *gd;
> +   unsigned int new_request = 0;
> +   unsigned int last_request = 0;
> +   int ret;
> +
> +   policy = (struct cpufreq_policy *) data;
> +   gd = policy->governor_data;
> +
> +   param.sched_priority = 50;
> +   ret = sched_setscheduler_nocheck(gd->task, SCHED_FIFO, );
> +   if (ret) {
> +   pr_warn("%s: failed to set SCHED_FIFO\n", __func__);
> +   do_exit(-EINVAL);
> +   } else {
> +   pr_debug("%s: kthread (%d) set to SCHED_FIFO\n",
> +   __func__, gd->task->pid);
> +   }
> +
> +   do {
> +   set_current_state(TASK_INTERRUPTIBLE);
> +   new_request = gd->requested_freq;
> +   if (new_request == last_request) {
> +   schedule();

Should we check kthread_should_stop() after
set_current_state(TASK_INTERRUPTIBLE), probably right before
schedule()? Something like:

   set_current_state(TASK_INTERRUPTIBLE);
   new_request = gd->requested_freq;
   if (new_request == last_request) {
   if (kthread_should_stop())
   break;
   schedule();
   } else {
   ...
   }

On the previous version of the scheduler-driver cpu frequency
selection I had the following:

<3>[ 1920.233598] INFO: task autotest:32443 blocked for more than 120 seconds.
<3>[ 1920.233625]   Not tainted 3.18.0-09696-g4312b25 #1
<3>[ 1920.233641] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
<6>[ 1920.233659] autotestD ffc0002057a0 0 32443
32403 0x0040
<0>[ 1920.233693] Call trace:
<4>[ 1920.233724] [] __switch_to+0x80/0x8c
<4>[ 1920.233748] [] __schedule+0x550/0x7d8
<4>[ 1920.233769] [] schedule+0x78/0x84
<4>[ 1920.233786] [] schedule_timeout+0x40/0x2ac
<4>[ 1920.233804] [] wait_for_common+0x154/0x18c
<4>[ 1920.233820] [] wait_for_completion+0x24/0x34
<4>[ 1920.233840] [] kthread_stop+0x130/0x22c
<4>[ 1920.233859] [] cpufreq_sched_setup+0x21c/0x308
<4>[ 1920.233881] [] __cpufreq_governor+0x114/0x1c8
<4>[ 1920.233901] [] cpufreq_set_policy+0x120/0x1b8
<4>[ 1920.233920] [] store_scaling_governor+0x8c/0xd4
<4>[ 1920.233937] [] store+0x98/0xd0
<4>[ 1920.233958] [] sysfs_kf_write+0x54/0x64
<4>[ 1920.233977] [] kernfs_fop_write+0x108/0x150
<4>[ 1920.233999] [] vfs_write+0xc4/0x1a0
<4>[ 1920.234018] [] SyS_write+0x60/0xb4
<4>[ 1920.234031] INFO: lockdep is turned off.
<6>[ 1920.234043]   taskPC stack   pid father
<6>[ 1920.234161] autotestD ffc0002057a0 0 32443
32403 0x0040
<0>[ 1920.234193] Call trace:
<4>[ 1920.234211] [] __switch_to+0x80/0x8c
<4>[ 1920.234232] [] __schedule+0x550/0x7d8
<4>[ 1920.234251] [] schedule+0x78/0x84
<4>[ 1920.234268] [] schedule_timeout+0x40/0x2ac
<4>[ 1920.234285] [] wait_for_common+0x154/0x18c
<4>[ 1920.234301] [] wait_for_completion+0x24/0x34
<4>[ 1920.234319] [] kthread_stop+0x130/0x22c
<4>[ 1920.234335] [] cpufreq_sched_setup+0x21c/0x308
<4>[ 1920.234355] [] __cpufreq_governor+0x114/0x1c8
<4>[ 1920.234375] [] cpufreq_set_policy+0x120/0x1b8
<4>[ 1920.234395] [] store_scaling_governor+0x8c/0xd4
<4>[ 1920.234413] [] store+0x98/0xd0
<4>[ 1920.234432] [] sysfs_kf_write+0x54/0x64
<4>[ 1920.234449] [] kernfs_fop_write+0x108/0x150
<4>[ 1920.234470] [] vfs_write+0xc4/0x1a0
<4>[ 1920.234489] [] SyS_write+0x60/0xb4

This happened while the kernel is switching from the sched governor to
the userspace governor. There's a race between kthread_stop() and
cpufreq_sched_thread(). On the previous version I was testing, I can
easily reproduce the lockup if I add a msleep(100) right before
set_current_state(TASK_INTERRUPTIBLE), and then switching between the
two governors through sysfs.

> +   } else {
> +   /*
> +* if the frequency thread sleeps while waiting to be
> +* unthrottled, start over to check for a newer 
> request
> +*/
> +   if (finish_last_request(gd))
> +   continue;
> +   last_request = new_request;
> +   cpufreq_sched_try_driver_target(policy, new_request);
> +   }
> +   } while (!kthread_should_stop());
> +
> +   

Re: [RFC 08/14] sched/tune: add detailed documentation

[Ricky Liang]

Hi Patrick,

Please find my replies inline.

On Thu, Sep 3, 2015 at 5:18 PM, Patrick Bellasi  wrote:
> On Wed, Sep 02, 2015 at 07:49:58AM +0100, Ricky Liang wrote:
>> Hi Patrick,
>
> Hi Ricky,
>
>> I wonder if this can replace the boost function in the interactive
>> governor [0], which is widely used in both Android and ChromeOS
>> kernels.
>
> In my view, one of the main goals of sched-DVFS is actually that to be
> a solid and generic replacement of different CPUFreq governors.
> Being driven by the scheduler, sched-DVFS can exploit information on
> CPU demand of active tasks in order to select the optimal Operating
> Performance Point (OPP) using a "proactive" approach instead of the
> "reactive" approach commonly used by existing governors.
>
> In the current implementation proposed by this RFC, SchedTune is just
> a simple mechanism on top of sched-DVFS to bias the selection of the
> OPP.
> In case a task is running for a limited amount of time at each of its
> (sporadic) activation, it does not contribute a CPU load which selects
> an higher OPP. Thus, the actual performance (i.e. time to completion)
> of that task depends on which other tasks are co-scheduled with it.
> If it has the chance to be scheduled on a loaded CPU it will run fast,
> to the contrary it will be slower when scheduled alone on a CPU
> running at the lowest OPP.
>
> If this task is (for whatever reason) "important" and should always
> complete an activation as soon as possible, the current situation is:
> a) we use the "performance" governor when we know that the task could
>be active, thus running the whole system in "race-to-idle" mode
> b) we use the "interactive" governor (if possible, since it is not not
>in mainline) and ensure that this task pokes the "boost" attribute
>when it is active
>
> Notice that, for both these solutions:
> 1) unless we pin the task on a specific set of CPUs, we must enable
>this governor for all the frequency domains since we do not know on
>which CPU the scheduler will end up to run the task
> 2) the tuning for a single task is likely to affect the whole system
>once the task as been started all the tasks are going to be boosted
>even when this task is not runnable
>
> SchedTune provides a "global tunable" which allows to get the same
> results as a) and b) with the main advantage that only the specific
> frequency domain where the task is RUNNING is boosted. Since we do not
> need to pin the task to get this result this can simplify (eventually)
> the modification required in user-space while still getting optimal
> performances for the task without compromising overall system
> consumption.
>
> AFAIU, regarding specifically the boost modes supported by the
> Interactive governor:
>
> 1) the "boost" tunable is substantially similar to setting to 100% the
>SchedTune boost value. Userspace is in charge to trigger the start
>and end of a boost period
>
> 2) the "boostpulse" tunable triggers a 100% boost.
>
>The main difference is that the Interactive governor resets the
>boost after a configurable time (usually 80ms) while in SchedTune
>the boost value is asserted until release by userspace.
>
>This has advantages and disadvantages. By using SchedTune the
>userspace has to release the boost explicitly. With the Interactive
>governor this is automatic but still the userspace has to defined a
>suitable timeout. However, this can be different for different
>tasks.
>
> 3) the "boost_input" tunable is just an hook exposed to kernel drivers
>which can generate input events expected to impact on user the
>experience.
>The actual implementation is just similar to the previous knob.
>
> IMHO the "boostpulse/input_pulse" tunables are a simple solution to
> the problem of running fast to get better UI interactive response.
> Indeed, the driver/task which generates the input event is not
> necessary the actual target of the load and/or user perceived
> response.
> Moreover, it boosts all the frequency domains independently from where
> the actual UI related workload is running.
>
> By exploiting scheduler information on the actual workload demand of
> some tasks, we could aim at a more effective solution which boost just
> the required CPUs and only when the task affecting the UI experience
> is actually running. This is what the "per-task" SchedTune boosting is
> trying to enable.
>
> I'm wondering if you could provide some example to better describe
> when the "boostpulse" tunables are used in ChromiumOS.
&

Re: [RFC 08/14] sched/tune: add detailed documentation

[Ricky Liang]

Hi Patrick,

Please find my replies inline.

On Thu, Sep 3, 2015 at 5:18 PM, Patrick Bellasi <patrick.bell...@arm.com> wrote:
> On Wed, Sep 02, 2015 at 07:49:58AM +0100, Ricky Liang wrote:
>> Hi Patrick,
>
> Hi Ricky,
>
>> I wonder if this can replace the boost function in the interactive
>> governor [0], which is widely used in both Android and ChromeOS
>> kernels.
>
> In my view, one of the main goals of sched-DVFS is actually that to be
> a solid and generic replacement of different CPUFreq governors.
> Being driven by the scheduler, sched-DVFS can exploit information on
> CPU demand of active tasks in order to select the optimal Operating
> Performance Point (OPP) using a "proactive" approach instead of the
> "reactive" approach commonly used by existing governors.
>
> In the current implementation proposed by this RFC, SchedTune is just
> a simple mechanism on top of sched-DVFS to bias the selection of the
> OPP.
> In case a task is running for a limited amount of time at each of its
> (sporadic) activation, it does not contribute a CPU load which selects
> an higher OPP. Thus, the actual performance (i.e. time to completion)
> of that task depends on which other tasks are co-scheduled with it.
> If it has the chance to be scheduled on a loaded CPU it will run fast,
> to the contrary it will be slower when scheduled alone on a CPU
> running at the lowest OPP.
>
> If this task is (for whatever reason) "important" and should always
> complete an activation as soon as possible, the current situation is:
> a) we use the "performance" governor when we know that the task could
>be active, thus running the whole system in "race-to-idle" mode
> b) we use the "interactive" governor (if possible, since it is not not
>in mainline) and ensure that this task pokes the "boost" attribute
>when it is active
>
> Notice that, for both these solutions:
> 1) unless we pin the task on a specific set of CPUs, we must enable
>this governor for all the frequency domains since we do not know on
>which CPU the scheduler will end up to run the task
> 2) the tuning for a single task is likely to affect the whole system
>once the task as been started all the tasks are going to be boosted
>even when this task is not runnable
>
> SchedTune provides a "global tunable" which allows to get the same
> results as a) and b) with the main advantage that only the specific
> frequency domain where the task is RUNNING is boosted. Since we do not
> need to pin the task to get this result this can simplify (eventually)
> the modification required in user-space while still getting optimal
> performances for the task without compromising overall system
> consumption.
>
> AFAIU, regarding specifically the boost modes supported by the
> Interactive governor:
>
> 1) the "boost" tunable is substantially similar to setting to 100% the
>SchedTune boost value. Userspace is in charge to trigger the start
>and end of a boost period
>
> 2) the "boostpulse" tunable triggers a 100% boost.
>
>The main difference is that the Interactive governor resets the
>boost after a configurable time (usually 80ms) while in SchedTune
>the boost value is asserted until release by userspace.
>
>This has advantages and disadvantages. By using SchedTune the
>userspace has to release the boost explicitly. With the Interactive
>governor this is automatic but still the userspace has to defined a
>suitable timeout. However, this can be different for different
>tasks.
>
> 3) the "boost_input" tunable is just an hook exposed to kernel drivers
>which can generate input events expected to impact on user the
>experience.
>The actual implementation is just similar to the previous knob.
>
> IMHO the "boostpulse/input_pulse" tunables are a simple solution to
> the problem of running fast to get better UI interactive response.
> Indeed, the driver/task which generates the input event is not
> necessary the actual target of the load and/or user perceived
> response.
> Moreover, it boosts all the frequency domains independently from where
> the actual UI related workload is running.
>
> By exploiting scheduler information on the actual workload demand of
> some tasks, we could aim at a more effective solution which boost just
> the required CPUs and only when the task affecting the UI experience
> is actually running. This is what the "per-task" SchedTune boosting is
> trying to enable.
>
> I'm wondering if you could provide some example to better describe
> when the "boostpulse" 

Re: [RFC,08/14] sched/tune: add detailed documentation

[Ricky Liang]

Hi Patrick,

I wonder if this can replace the boost function in the interactive governor [0],
which is widely used in both Android and ChromeOS kernels.

My understanding is that the boost in interactive governor is to simply raise
the OPP on selected cores. The SchedTune boost works by adding a margin to the
original load of a task which makes the kernel think that the task is more
demanding than it actually is. My intuition was that they work differently and
could cause different reaction in the kernel.

I feel that the per-task cgroup ScheTune boost should work as expected as it
only boosts a set of tasks and make them appear relatively high demanding
comparing to other tasks. But if the ScheTune boost is applied globally to boost
all the tasks in the system, will it cause unnecessary task migrations as all
the tasks appear to be high demanding to the kernel?

Specifically, my questions is: When the global SchedTune boost is enabled in a
on-demand manner, is it possible that a light task gets migrated to the big
core, and in turn kicks out a heavy task originally on that core? I'm wondering
whether global SchedTune boost could result in a "priority inversion" causing
the heavy task to run on the little core and the light task to run on the big
core.

[0]: 
https://android.googlesource.com/kernel/common.git/+/android-3.18/drivers/cpufreq/cpufreq_interactive.c

Thanks,
Ricky

On Wed, Aug 19, 2015 at 07:47:18PM +0100, Patrick Bellasi wrote:
> The topic of a single simple power-performance tunable, that is wholly
> scheduler centric, and has well defined and predictable properties has
> come up on several occasions in the past. With techniques such as a
> scheduler driven DVFS, we now have a good framework for implementing
> such a tunable.
> 
> This patch provides a detailed description of the motivations and design
> decisions behind the implementation of the SchedTune.
> 
> cc: Jonathan Corbet 
> cc: linux-...@vger.kernel.org
> Signed-off-by: Patrick Bellasi 
> 
> ---
> Documentation/scheduler/sched-tune.txt | 367 +
>  1 file changed, 367 insertions(+)
>  create mode 100644 Documentation/scheduler/sched-tune.txt
> 
> diff --git a/Documentation/scheduler/sched-tune.txt 
> b/Documentation/scheduler/sched-tune.txt
> new file mode 100644
> index 000..cb795e6
> --- /dev/null
> +++ b/Documentation/scheduler/sched-tune.txt
> @@ -0,0 +1,367 @@
> + Central, scheduler-driven, power-performance control
> +   (EXPERIMENTAL)
> +
> +Abstract
> +
> +
> +The topic of a single simple power-performance tunable, that is wholly
> +scheduler centric, and has well defined and predictable properties has come 
> up
> +on several occasions in the past [1,2]. With techniques such as a scheduler
> +driven DVFS [3], we now have a good framework for implementing such a 
> tunable.
> +This document describes the overall ideas behind its design and 
> implementation.
> +
> +
> +Table of Contents
> +=
> +
> +1. Motivation
> +2. Introduction
> +3. Signal Boosting Strategy
> +4. OPP selection using boosted CPU utilization
> +5. Per task group boosting
> +6. Question and Answers
> +   - What about "auto" mode?
> +   - What about boosting on a congested system?
> +   - How CPUs are boosted when we have tasks with multiple boost values?
> +7. References
> +
> +
> +1. Motivation
> +=
> +
> +Sched-DVFS [3] is a new event-driven cpufreq governor which allows the
> +scheduler to select the optimal DVFS operating point (OPP) for running a task
> +allocated to a CPU. The introduction of sched-DVFS enables running workloads 
> at
> +the most energy efficient OPPs.
> +
> +However, sometimes it may be desired to intentionally boost the performance 
> of
> +a workload even if that could imply a reasonable increase in energy
> +consumption. For example, in order to reduce the response time of a task, we
> +may want to run the task at a higher OPP than the one that is actually 
> required
> +by it's CPU bandwidth demand.
> +
> +This last requirement is especially important if we consider that one of the
> +main goals of the sched-DVFS component is to replace all currently available
> +CPUFreq policies. Since sched-DVFS is event based, as opposed to the sampling
> +driven governors we currently have, it is already more responsive at 
> selecting
> +the optimal OPP to run tasks allocated to a CPU. However, just tracking the
> +actual task load demand may not be enough from a performance standpoint.  For
> +example, it is not possible to get behaviors similar to those provided by the
> +"performance" and "interactive" CPUFreq governors.
> +
> +This document describes an implementation of a tunable, stacked on top of the
> +sched-DVFS which extends its functionality to support task performance
> +boosting.
> +
> +By "performance boosting" we mean the reduction of the time required to
> +complete a task activation, i.e. the time elapsed from a 

Re: [RFC,08/14] sched/tune: add detailed documentation

[Ricky Liang]

Hi Patrick,

I wonder if this can replace the boost function in the interactive governor [0],
which is widely used in both Android and ChromeOS kernels.

My understanding is that the boost in interactive governor is to simply raise
the OPP on selected cores. The SchedTune boost works by adding a margin to the
original load of a task which makes the kernel think that the task is more
demanding than it actually is. My intuition was that they work differently and
could cause different reaction in the kernel.

I feel that the per-task cgroup ScheTune boost should work as expected as it
only boosts a set of tasks and make them appear relatively high demanding
comparing to other tasks. But if the ScheTune boost is applied globally to boost
all the tasks in the system, will it cause unnecessary task migrations as all
the tasks appear to be high demanding to the kernel?

Specifically, my questions is: When the global SchedTune boost is enabled in a
on-demand manner, is it possible that a light task gets migrated to the big
core, and in turn kicks out a heavy task originally on that core? I'm wondering
whether global SchedTune boost could result in a "priority inversion" causing
the heavy task to run on the little core and the light task to run on the big
core.

[0]: 
https://android.googlesource.com/kernel/common.git/+/android-3.18/drivers/cpufreq/cpufreq_interactive.c

Thanks,
Ricky

On Wed, Aug 19, 2015 at 07:47:18PM +0100, Patrick Bellasi wrote:
> The topic of a single simple power-performance tunable, that is wholly
> scheduler centric, and has well defined and predictable properties has
> come up on several occasions in the past. With techniques such as a
> scheduler driven DVFS, we now have a good framework for implementing
> such a tunable.
> 
> This patch provides a detailed description of the motivations and design
> decisions behind the implementation of the SchedTune.
> 
> cc: Jonathan Corbet 
> cc: linux-...@vger.kernel.org
> Signed-off-by: Patrick Bellasi 
> 
> ---
> Documentation/scheduler/sched-tune.txt | 367 +
>  1 file changed, 367 insertions(+)
>  create mode 100644 Documentation/scheduler/sched-tune.txt
> 
> diff --git a/Documentation/scheduler/sched-tune.txt 
> b/Documentation/scheduler/sched-tune.txt
> new file mode 100644
> index 000..cb795e6
> --- /dev/null
> +++ b/Documentation/scheduler/sched-tune.txt
> @@ -0,0 +1,367 @@
> + Central, scheduler-driven, power-performance control
> +   (EXPERIMENTAL)
> +
> +Abstract
> +
> +
> +The topic of a single simple power-performance tunable, that is wholly
> +scheduler centric, and has well defined and predictable properties has come 
> up
> +on several occasions in the past [1,2]. With techniques such as a scheduler
> +driven DVFS [3], we now have a good framework for implementing such a 
> tunable.
> +This document describes the overall ideas behind its design and 
> implementation.
> +
> +
> +Table of Contents
> +=
> +
> +1. Motivation
> +2. Introduction
> +3. Signal Boosting Strategy
> +4. OPP selection using boosted CPU utilization
> +5. Per task group boosting
> +6. Question and Answers
> +   - What about "auto" mode?
> +   - What about boosting on a congested system?
> +   - How CPUs are boosted when we have tasks with multiple boost values?
> +7. References
> +
> +
> +1. Motivation
> +=
> +
> +Sched-DVFS [3] is a new event-driven cpufreq governor which allows the
> +scheduler to select the optimal DVFS operating point (OPP) for running a task
> +allocated to a CPU. The introduction of sched-DVFS enables running workloads 
> at
> +the most energy efficient OPPs.
> +
> +However, sometimes it may be desired to intentionally boost the performance 
> of
> +a workload even if that could imply a reasonable increase in energy
> +consumption. For example, in order to reduce the response time of a task, we
> +may want to run the task at a higher OPP than the one that is actually 
> required
> +by it's CPU bandwidth demand.
> +
> +This last requirement is especially important if we consider that one of the
> +main goals of the sched-DVFS component is to replace all currently available
> +CPUFreq policies. Since sched-DVFS is event based, as opposed to the sampling
> +driven governors we currently have, it is already more responsive at 
> selecting
> +the optimal OPP to run tasks allocated to a CPU. However, just tracking the
> +actual task load demand may not be enough from a performance standpoint.  For
> +example, it is not possible to get behaviors similar to those provided by the
> +"performance" and "interactive" CPUFreq governors.
> +
> +This document describes an implementation of a tunable, stacked on top of the
> +sched-DVFS which extends its functionality to support task performance
> +boosting.
> +
> +By "performance boosting" we mean the reduction of the time required to
> +complete a task 

Re: [PATCH v3 3/5] usb: phy: add usb3.0 phy driver for mt65xx SoCs

[Ricky Liang]

Hi Chungfeng,

Comments inline.

On Wed, Jul 22, 2015 at 10:05 PM, Chunfeng Yun
 wrote:
> support usb3.0 phy of mt65xx SoCs
>
> Signed-off-by: Chunfeng Yun 
> ---
>  drivers/phy/Kconfig   |   9 +
>  drivers/phy/Makefile  |   1 +
>  drivers/phy/phy-mt65xx-usb3.c | 426 
> ++
>  3 files changed, 436 insertions(+)
>  create mode 100644 drivers/phy/phy-mt65xx-usb3.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index c0e6ede..019cf8b 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -193,6 +193,15 @@ config PHY_HIX5HD2_SATA
> help
>   Support for SATA PHY on Hisilicon hix5hd2 Soc.
>
> +config PHY_MT65XX_USB3
> +   tristate "Mediatek USB3.0 PHY Driver"
> +   depends on ARCH_MEDIATEK && OF
> +   select GENERIC_PHY
> +   help
> + Say 'Y' here to add support for Mediatek USB3.0 PHY driver
> + for mt65xx SoCs. it supports two usb2.0 ports and
> + one usb3.0 port.
> +
>  config PHY_SUN4I_USB
> tristate "Allwinner sunxi SoC USB PHY driver"
> depends on ARCH_SUNXI && HAS_IOMEM && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index f344e1b..3ceff2a 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_TI_PIPE3)+= 
> phy-ti-pipe3.o
>  obj-$(CONFIG_TWL4030_USB)  += phy-twl4030-usb.o
>  obj-$(CONFIG_PHY_EXYNOS5250_SATA)  += phy-exynos5250-sata.o
>  obj-$(CONFIG_PHY_HIX5HD2_SATA) += phy-hix5hd2-sata.o
> +obj-$(CONFIG_PHY_MT65XX_USB3)  += phy-mt65xx-usb3.o
>  obj-$(CONFIG_PHY_SUN4I_USB)+= phy-sun4i-usb.o
>  obj-$(CONFIG_PHY_SUN9I_USB)+= phy-sun9i-usb.o
>  obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-exynos-usb2.o
> diff --git a/drivers/phy/phy-mt65xx-usb3.c b/drivers/phy/phy-mt65xx-usb3.c
> new file mode 100644
> index 000..5da4534
> --- /dev/null
> +++ b/drivers/phy/phy-mt65xx-usb3.c
> @@ -0,0 +1,426 @@
> +/*
> + * Copyright (c) 2015 MediaTek Inc.
> + * Author: Chunfeng.Yun 
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * for sifslv2 register
> + * relative to USB3_SIF2_BASE base address
> + */
> +#define SSUSB_SIFSLV_SPLLC (0x)
> +#define SSUSB_SIFSLV_U2PHY_COM_BASE(0x0800)
> +#define SSUSB_SIFSLV_U3PHYD_BASE   (0x0900)
> +#define SSUSB_USB30_PHYA_SIV_B_BASE(0x0b00)
> +#define SSUSB_SIFSLV_U3PHYA_DA_BASE(0x0c00)

You don't need () here. Same for all following numeric constants.

> +
> +/*port1 refs. +0x800(refer to port0)*/
> +#define U3P_PORT_INTERVAL (0x800)  /*based on port0 */
> +#define U3P_PHY_DELTA(index) ((U3P_PORT_INTERVAL) * (index))

Indent with tab. It might also be a good idea to align the
indentations of all the macros.

> +
> +#define U3P_USBPHYACR0 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x)
> +#define PA0_RG_U2PLL_FORCE_ON  (0x1 << 15)

Use BIT() instead? Same for all following (0x1 << xx) macros.

> +
> +#define U3P_USBPHYACR2 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0008)
> +#define PA2_RG_SIF_U2PLL_FORCE_EN  (0x1 << 18)
> +
> +#define U3P_USBPHYACR5 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0014)
> +#define PA5_RG_U2_HSTX_SRCTRL  (0x7 << 12)
> +#define PA5_RG_U2_HSTX_SRCTRL_VAL(x)   ((0x7 & (x)) << 12)
> +#define PA5_RG_U2_HS_100U_U3_EN(0x1 << 11)
> +
> +#define U3P_USBPHYACR6 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0018)
> +#define PA6_RG_U2_ISO_EN   (0x1 << 31)
> +#define PA6_RG_U2_BC11_SW_EN   (0x1 << 23)
> +#define PA6_RG_U2_OTG_VBUSCMP_EN   (0x1 << 20)
> +
> +#define U3P_U2PHYACR4  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0020)
> +#define P2C_RG_USB20_GPIO_CTL  (0x1 << 9)
> +#define P2C_USB20_GPIO_MODE(0x1 << 8)
> +#define P2C_U2_GPIO_CTR_MSK(P2C_RG_USB20_GPIO_CTL | P2C_USB20_GPIO_MODE)
> +
> +#define U3D_U2PHYDCR0  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0060)
> +#define P2C_RG_SIF_U2PLL_FORCE_ON  (0x1 << 24)
> +
> +#define U3P_U2PHYDTM0  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0068)
> +#define P2C_FORCE_UART_EN  (0x1 << 26)
> +#define P2C_FORCE_DATAIN   (0x1 << 23)
> +#define P2C_FORCE_DM_PULLDOWN  (0x1 << 21)
> +#define P2C_FORCE_DP_PULLDOWN  (0x1 << 20)
> +#define P2C_FORCE_XCVRSEL  (0x1 << 19)
> +#define P2C_FORCE_SUSPENDM (0x1 << 18)
> +#define P2C_FORCE_TERMSEL  (0x1 << 17)
> +#define P2C_RG_DATAIN  

Re: [PATCH v3 3/5] usb: phy: add usb3.0 phy driver for mt65xx SoCs

[Ricky Liang]

Hi Chungfeng,

Comments inline.

On Wed, Jul 22, 2015 at 10:05 PM, Chunfeng Yun
chunfeng@mediatek.com wrote:
 support usb3.0 phy of mt65xx SoCs

 Signed-off-by: Chunfeng Yun chunfeng@mediatek.com
 ---
  drivers/phy/Kconfig   |   9 +
  drivers/phy/Makefile  |   1 +
  drivers/phy/phy-mt65xx-usb3.c | 426 
 ++
  3 files changed, 436 insertions(+)
  create mode 100644 drivers/phy/phy-mt65xx-usb3.c

 diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
 index c0e6ede..019cf8b 100644
 --- a/drivers/phy/Kconfig
 +++ b/drivers/phy/Kconfig
 @@ -193,6 +193,15 @@ config PHY_HIX5HD2_SATA
 help
   Support for SATA PHY on Hisilicon hix5hd2 Soc.

 +config PHY_MT65XX_USB3
 +   tristate Mediatek USB3.0 PHY Driver
 +   depends on ARCH_MEDIATEK  OF
 +   select GENERIC_PHY
 +   help
 + Say 'Y' here to add support for Mediatek USB3.0 PHY driver
 + for mt65xx SoCs. it supports two usb2.0 ports and
 + one usb3.0 port.
 +
  config PHY_SUN4I_USB
 tristate Allwinner sunxi SoC USB PHY driver
 depends on ARCH_SUNXI  HAS_IOMEM  OF
 diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
 index f344e1b..3ceff2a 100644
 --- a/drivers/phy/Makefile
 +++ b/drivers/phy/Makefile
 @@ -22,6 +22,7 @@ obj-$(CONFIG_TI_PIPE3)+= 
 phy-ti-pipe3.o
  obj-$(CONFIG_TWL4030_USB)  += phy-twl4030-usb.o
  obj-$(CONFIG_PHY_EXYNOS5250_SATA)  += phy-exynos5250-sata.o
  obj-$(CONFIG_PHY_HIX5HD2_SATA) += phy-hix5hd2-sata.o
 +obj-$(CONFIG_PHY_MT65XX_USB3)  += phy-mt65xx-usb3.o
  obj-$(CONFIG_PHY_SUN4I_USB)+= phy-sun4i-usb.o
  obj-$(CONFIG_PHY_SUN9I_USB)+= phy-sun9i-usb.o
  obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-exynos-usb2.o
 diff --git a/drivers/phy/phy-mt65xx-usb3.c b/drivers/phy/phy-mt65xx-usb3.c
 new file mode 100644
 index 000..5da4534
 --- /dev/null
 +++ b/drivers/phy/phy-mt65xx-usb3.c
 @@ -0,0 +1,426 @@
 +/*
 + * Copyright (c) 2015 MediaTek Inc.
 + * Author: Chunfeng.Yun chunfeng@mediatek.com
 + *
 + * This software is licensed under the terms of the GNU General Public
 + * License version 2, as published by the Free Software Foundation, and
 + * may be copied, distributed, and modified under those terms.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + */
 +
 +#include linux/clk.h
 +#include linux/delay.h
 +#include linux/io.h
 +#include linux/module.h
 +#include linux/of_address.h
 +#include linux/of_device.h
 +#include linux/of_gpio.h
 +#include linux/of.h
 +#include linux/phy/phy.h
 +#include linux/platform_device.h
 +#include linux/pm_runtime.h
 +#include linux/regulator/consumer.h
 +#include linux/resource.h
 +
 +/*
 + * for sifslv2 register
 + * relative to USB3_SIF2_BASE base address
 + */
 +#define SSUSB_SIFSLV_SPLLC (0x)
 +#define SSUSB_SIFSLV_U2PHY_COM_BASE(0x0800)
 +#define SSUSB_SIFSLV_U3PHYD_BASE   (0x0900)
 +#define SSUSB_USB30_PHYA_SIV_B_BASE(0x0b00)
 +#define SSUSB_SIFSLV_U3PHYA_DA_BASE(0x0c00)

You don't need () here. Same for all following numeric constants.

 +
 +/*port1 refs. +0x800(refer to port0)*/
 +#define U3P_PORT_INTERVAL (0x800)  /*based on port0 */
 +#define U3P_PHY_DELTA(index) ((U3P_PORT_INTERVAL) * (index))

Indent with tab. It might also be a good idea to align the
indentations of all the macros.

 +
 +#define U3P_USBPHYACR0 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x)
 +#define PA0_RG_U2PLL_FORCE_ON  (0x1  15)

Use BIT() instead? Same for all following (0x1  xx) macros.

 +
 +#define U3P_USBPHYACR2 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0008)
 +#define PA2_RG_SIF_U2PLL_FORCE_EN  (0x1  18)
 +
 +#define U3P_USBPHYACR5 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0014)
 +#define PA5_RG_U2_HSTX_SRCTRL  (0x7  12)
 +#define PA5_RG_U2_HSTX_SRCTRL_VAL(x)   ((0x7  (x))  12)
 +#define PA5_RG_U2_HS_100U_U3_EN(0x1  11)
 +
 +#define U3P_USBPHYACR6 (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0018)
 +#define PA6_RG_U2_ISO_EN   (0x1  31)
 +#define PA6_RG_U2_BC11_SW_EN   (0x1  23)
 +#define PA6_RG_U2_OTG_VBUSCMP_EN   (0x1  20)
 +
 +#define U3P_U2PHYACR4  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0020)
 +#define P2C_RG_USB20_GPIO_CTL  (0x1  9)
 +#define P2C_USB20_GPIO_MODE(0x1  8)
 +#define P2C_U2_GPIO_CTR_MSK(P2C_RG_USB20_GPIO_CTL | P2C_USB20_GPIO_MODE)
 +
 +#define U3D_U2PHYDCR0  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0060)
 +#define P2C_RG_SIF_U2PLL_FORCE_ON  (0x1  24)
 +
 +#define U3P_U2PHYDTM0  (SSUSB_SIFSLV_U2PHY_COM_BASE + 0x0068)
 +#define P2C_FORCE_UART_EN  (0x1  26)
 +#define P2C_FORCE_DATAIN   (0x1  23)
 +#define P2C_FORCE_DM_PULLDOWN  (0x1  21)
 +#define P2C_FORCE_DP_PULLDOWN  (0x1  20)
 +#define P2C_FORCE_XCVRSEL  (0x1  19)
 +#define 

[PATCH v3] clk: mediatek: Initialize clk_init_data

[Ricky Liang]

The variable init (struct clk_init_data) is allocated on the stack.
We weren't initializing the .flags field, so it contains random junk,
which can cause all kinds of interesting issues when the flags are
parsed by clk_register.

Signed-off-by: Ricky Liang 
---
 drivers/clk/mediatek/clk-gate.c | 2 +-
 drivers/clk/mediatek/clk-pll.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mediatek/clk-gate.c b/drivers/clk/mediatek/clk-gate.c
index 9d77ee3..5702036 100644
--- a/drivers/clk/mediatek/clk-gate.c
+++ b/drivers/clk/mediatek/clk-gate.c
@@ -109,7 +109,7 @@ struct clk *mtk_clk_register_gate(
 {
struct mtk_clk_gate *cg;
struct clk *clk;
-   struct clk_init_data init;
+   struct clk_init_data init = {};
 
cg = kzalloc(sizeof(*cg), GFP_KERNEL);
if (!cg)
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 66154ca..44409e9 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -268,7 +268,7 @@ static struct clk *mtk_clk_register_pll(const struct 
mtk_pll_data *data,
void __iomem *base)
 {
struct mtk_clk_pll *pll;
-   struct clk_init_data init;
+   struct clk_init_data init = {};
struct clk *clk;
const char *parent_name = "clk26m";
 
-- 
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] clk: mediatek: Initialize clk_init_data

[Ricky Liang]

Hi Sascha,

Sure. I can fix clk-gate.c as well. New patch on the way...

-Ricky

On Mon, May 18, 2015 at 1:54 PM, Sascha Hauer  wrote:
> Hi Ricky,
>
> On Mon, May 18, 2015 at 11:41:49AM +0800, Ricky Liang wrote:
>> The variable init (struct clk_init_data) is allocated on the stack.
>> We weren't initializing the .flags field, so it contains random junk,
>> which can cause all kinds of interesting issues when the flags are
>> parsed by clk_register.
>
> It seems we have the same problem in clk-gate.c aswell. We do initialize
> do .flags field there, so this is no urgent problem, but we might get a
> real problem when additional fields are added to struct clk_init_data.
> Care to fix that aswell along with this patch?
>
> Sascha
>
> --
> Pengutronix e.K.   | |
> Industrial Linux Solutions | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] clk: mediatek: Initialize clk_init_data

[Ricky Liang]

Hi Sascha,

Sure. I can fix clk-gate.c as well. New patch on the way...

-Ricky

On Mon, May 18, 2015 at 1:54 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
 Hi Ricky,

 On Mon, May 18, 2015 at 11:41:49AM +0800, Ricky Liang wrote:
 The variable init (struct clk_init_data) is allocated on the stack.
 We weren't initializing the .flags field, so it contains random junk,
 which can cause all kinds of interesting issues when the flags are
 parsed by clk_register.

 It seems we have the same problem in clk-gate.c aswell. We do initialize
 do .flags field there, so this is no urgent problem, but we might get a
 real problem when additional fields are added to struct clk_init_data.
 Care to fix that aswell along with this patch?

 Sascha

 --
 Pengutronix e.K.   | |
 Industrial Linux Solutions | http://www.pengutronix.de/  |
 Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
 Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3] clk: mediatek: Initialize clk_init_data

[Ricky Liang]

The variable init (struct clk_init_data) is allocated on the stack.
We weren't initializing the .flags field, so it contains random junk,
which can cause all kinds of interesting issues when the flags are
parsed by clk_register.

Signed-off-by: Ricky Liang jcli...@chromium.org
---
 drivers/clk/mediatek/clk-gate.c | 2 +-
 drivers/clk/mediatek/clk-pll.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mediatek/clk-gate.c b/drivers/clk/mediatek/clk-gate.c
index 9d77ee3..5702036 100644
--- a/drivers/clk/mediatek/clk-gate.c
+++ b/drivers/clk/mediatek/clk-gate.c
@@ -109,7 +109,7 @@ struct clk *mtk_clk_register_gate(
 {
struct mtk_clk_gate *cg;
struct clk *clk;
-   struct clk_init_data init;
+   struct clk_init_data init = {};
 
cg = kzalloc(sizeof(*cg), GFP_KERNEL);
if (!cg)
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 66154ca..44409e9 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -268,7 +268,7 @@ static struct clk *mtk_clk_register_pll(const struct 
mtk_pll_data *data,
void __iomem *base)
 {
struct mtk_clk_pll *pll;
-   struct clk_init_data init;
+   struct clk_init_data init = {};
struct clk *clk;
const char *parent_name = clk26m;
 
-- 
2.1.2

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

[PATCH v2] clk: mediatek: Initialize clk_init_data

[Ricky Liang]

The variable init (struct clk_init_data) is allocated on the stack.
We weren't initializing the .flags field, so it contains random junk,
which can cause all kinds of interesting issues when the flags are
parsed by clk_register.

Signed-off-by: Ricky Liang 
---
 drivers/clk/mediatek/clk-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 66154ca..44409e9 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -268,7 +268,7 @@ static struct clk *mtk_clk_register_pll(const struct 
mtk_pll_data *data,
void __iomem *base)
 {
struct mtk_clk_pll *pll;
-   struct clk_init_data init;
+   struct clk_init_data init = {};
struct clk *clk;
const char *parent_name = "clk26m";
 
-- 
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] clk: mediatek: Initialize clk_init_data

[Ricky Liang]

The variable init (struct clk_init_data) is allocated on the stack.
We weren't initializing the .flags field, so it contains random junk,
which can cause all kinds of interesting issues when the flags are
parsed by clk_register.

Signed-off-by: Ricky Liang jcli...@chromium.org
---
 drivers/clk/mediatek/clk-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 66154ca..44409e9 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -268,7 +268,7 @@ static struct clk *mtk_clk_register_pll(const struct 
mtk_pll_data *data,
void __iomem *base)
 {
struct mtk_clk_pll *pll;
-   struct clk_init_data init;
+   struct clk_init_data init = {};
struct clk *clk;
const char *parent_name = clk26m;
 
-- 
2.1.2

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

[PATCH] clk: mediatek: Initialize clk flags

[Ricky Liang]

The variable init (struct clk_init_data) is allocated on the stack.
We weren't initializing the .flags field, so it contains random junk,
which can cause all kinds of interesting issues when the flags are
parsed by clk_register.

The best solution seems to just pass on the flags passed in to our
clk_register wrappers.

Signed-off-by: Ricky Liang 
---
 drivers/clk/mediatek/clk-pll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 66154ca..72fe307 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -289,6 +289,7 @@ static struct clk *mtk_clk_register_pll(const struct 
mtk_pll_data *data,
init.ops = _pll_ops;
init.parent_names = _name;
init.num_parents = 1;
+   init.flags = data->flags;
 
clk = clk_register(NULL, >hw);
 
-- 
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Ricky Liang]

On Mon, Apr 20, 2015 at 5:25 PM, pi-cheng.chen  wrote:
> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for
> intermediate clock source switching. It is based on v11 of common clock 
> support
> for Mediatek MT8135 and MT8173[1]
>
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-April/335967.html
>
> Changes in v2:
> - Remove use of .determine_rate callback
>
> Signed-off-by: pi-cheng.chen 
> ---
>  drivers/clk/mediatek/Makefile  |   2 +-
>  drivers/clk/mediatek/clk-cpumux.c  | 122 
> +
>  drivers/clk/mediatek/clk-cpumux.h  |  31 +
>  drivers/clk/mediatek/clk-mt8173.c  |  23 +++
>  include/dt-bindings/clock/mt8173-clk.h |   4 +-
>  5 files changed, 180 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/clk/mediatek/clk-cpumux.c
>  create mode 100644 drivers/clk/mediatek/clk-cpumux.h
>
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index 8e4b2a4..299917a 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -1,4 +1,4 @@
> -obj-y += clk-mtk.o clk-pll.o clk-gate.o
> +obj-y += clk-mtk.o clk-pll.o clk-gate.o clk-cpumux.o
>  obj-$(CONFIG_RESET_CONTROLLER) += reset.o
>  obj-y += clk-mt8135.o
>  obj-y += clk-mt8173.o
> diff --git a/drivers/clk/mediatek/clk-cpumux.c 
> b/drivers/clk/mediatek/clk-cpumux.c
> new file mode 100644
> index 000..dc14074
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-cpumux.c
> @@ -0,0 +1,122 @@
> +/*
> + * Copyright (c) 2015 Linaro Ltd.
> + * Author: Pi-Cheng Chen 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "clk-mtk.h"
> +#include "clk-cpumux.h"
> +
> +static inline struct mtk_clk_cpumux *to_clk_mux(struct clk_hw *_hw)
> +{
> +   return container_of(_hw, struct mtk_clk_cpumux, hw);
> +}
> +
> +static u8 clk_cpumux_get_parent(struct clk_hw *hw)
> +{
> +   struct mtk_clk_cpumux *mux = to_clk_mux(hw);
> +   int num_parents = __clk_get_num_parents(hw->clk);
> +   u32 val;
> +
> +   regmap_read(mux->regmap, mux->reg, );
> +
> +   val >>= mux->shift;
> +   val &= mux->mask;
> +
> +   if (val >= num_parents)
> +   return -EINVAL;
> +
> +   return val;
> +}
> +
> +static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +   struct mtk_clk_cpumux *mux = to_clk_mux(hw);
> +   u32 mask, val;
> +
> +   val = index << mux->shift;
> +   mask = mux->mask << mux->shift;
> +
> +   return regmap_update_bits(mux->regmap, mux->reg, mask, val);
> +}
> +
> +static struct clk_ops clk_cpumux_ops = {
> +   .get_parent = clk_cpumux_get_parent,
> +   .set_parent = clk_cpumux_set_parent,
> +};
> +
> +static struct clk *mtk_clk_register_cpumux(const struct mtk_composite *mux,
> +  struct regmap *regmap)
> +{
> +   struct mtk_clk_cpumux *cpumux;
> +   struct clk *clk;
> +   struct clk_init_data init;
> +
> +   cpumux = kzalloc(sizeof(*cpumux), GFP_KERNEL);
> +   if (!cpumux)
> +   return ERR_PTR(-ENOMEM);
> +
> +   init.name = mux->name;
> +   init.ops = _cpumux_ops;
> +   init.parent_names = mux->parent_names;
> +   init.num_parents = mux->num_parents;

To be safe, please set

  init.flags = mux->flags;

here

> +
> +   cpumux->reg = mux->mux_reg;
> +   cpumux->shift = mux->mux_shift;
> +   cpumux->mask = (BIT(mux->mux_width) - 1);
> +   cpumux->regmap = regmap;
> +   cpumux->hw.init = 
> +
> +   clk = clk_register(NULL, >hw);
> +   if (IS_ERR(clk))
> +   kfree(cpumux);
> +
> +   return clk;
> +}
> +
> +int mtk_clk_register_cpumuxes(struct device_node *node,
> + const struct mtk_composite *clks, int num,
> + struct clk_onecell_data *clk_data)
> +{
> +   int i;
> +   struct clk *clk;
> +   struct regmap *regmap;
> +
> +   if (!clk_data)
> +   return -ENOMEM;
> +
> +   regmap = syscon_node_to_regmap(node);
> +   if (IS_ERR(regmap)) {
> +   pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
> +  PTR_ERR(regmap));
> +   return PTR_ERR(regmap);
> +   }
> +
> +   for (i = 0; i < num; i++) {
> +   const struct mtk_composite *mux = [i];
> +
> +   clk = mtk_clk_register_cpumux(mux, regmap);
> +   if (IS_ERR(clk)) {
> +   

Re: [PATCH v2] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Ricky Liang]

On Mon, Apr 20, 2015 at 5:25 PM, pi-cheng.chen pi-cheng.c...@linaro.org wrote:
 This patch adds CPU mux clocks which are used by Mediatek cpufreq driver for
 intermediate clock source switching. It is based on v11 of common clock 
 support
 for Mediatek MT8135 and MT8173[1]

 [1] 
 http://lists.infradead.org/pipermail/linux-arm-kernel/2015-April/335967.html

 Changes in v2:
 - Remove use of .determine_rate callback

 Signed-off-by: pi-cheng.chen pi-cheng.c...@linaro.org
 ---
  drivers/clk/mediatek/Makefile  |   2 +-
  drivers/clk/mediatek/clk-cpumux.c  | 122 
 +
  drivers/clk/mediatek/clk-cpumux.h  |  31 +
  drivers/clk/mediatek/clk-mt8173.c  |  23 +++
  include/dt-bindings/clock/mt8173-clk.h |   4 +-
  5 files changed, 180 insertions(+), 2 deletions(-)
  create mode 100644 drivers/clk/mediatek/clk-cpumux.c
  create mode 100644 drivers/clk/mediatek/clk-cpumux.h

 diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
 index 8e4b2a4..299917a 100644
 --- a/drivers/clk/mediatek/Makefile
 +++ b/drivers/clk/mediatek/Makefile
 @@ -1,4 +1,4 @@
 -obj-y += clk-mtk.o clk-pll.o clk-gate.o
 +obj-y += clk-mtk.o clk-pll.o clk-gate.o clk-cpumux.o
  obj-$(CONFIG_RESET_CONTROLLER) += reset.o
  obj-y += clk-mt8135.o
  obj-y += clk-mt8173.o
 diff --git a/drivers/clk/mediatek/clk-cpumux.c 
 b/drivers/clk/mediatek/clk-cpumux.c
 new file mode 100644
 index 000..dc14074
 --- /dev/null
 +++ b/drivers/clk/mediatek/clk-cpumux.c
 @@ -0,0 +1,122 @@
 +/*
 + * Copyright (c) 2015 Linaro Ltd.
 + * Author: Pi-Cheng Chen pi-cheng.c...@linaro.org
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +
 +#include linux/clk.h
 +#include linux/clk-provider.h
 +#include linux/mfd/syscon.h
 +#include linux/slab.h
 +
 +#include clk-mtk.h
 +#include clk-cpumux.h
 +
 +static inline struct mtk_clk_cpumux *to_clk_mux(struct clk_hw *_hw)
 +{
 +   return container_of(_hw, struct mtk_clk_cpumux, hw);
 +}
 +
 +static u8 clk_cpumux_get_parent(struct clk_hw *hw)
 +{
 +   struct mtk_clk_cpumux *mux = to_clk_mux(hw);
 +   int num_parents = __clk_get_num_parents(hw-clk);
 +   u32 val;
 +
 +   regmap_read(mux-regmap, mux-reg, val);
 +
 +   val = mux-shift;
 +   val = mux-mask;
 +
 +   if (val = num_parents)
 +   return -EINVAL;
 +
 +   return val;
 +}
 +
 +static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
 +{
 +   struct mtk_clk_cpumux *mux = to_clk_mux(hw);
 +   u32 mask, val;
 +
 +   val = index  mux-shift;
 +   mask = mux-mask  mux-shift;
 +
 +   return regmap_update_bits(mux-regmap, mux-reg, mask, val);
 +}
 +
 +static struct clk_ops clk_cpumux_ops = {
 +   .get_parent = clk_cpumux_get_parent,
 +   .set_parent = clk_cpumux_set_parent,
 +};
 +
 +static struct clk *mtk_clk_register_cpumux(const struct mtk_composite *mux,
 +  struct regmap *regmap)
 +{
 +   struct mtk_clk_cpumux *cpumux;
 +   struct clk *clk;
 +   struct clk_init_data init;
 +
 +   cpumux = kzalloc(sizeof(*cpumux), GFP_KERNEL);
 +   if (!cpumux)
 +   return ERR_PTR(-ENOMEM);
 +
 +   init.name = mux-name;
 +   init.ops = clk_cpumux_ops;
 +   init.parent_names = mux-parent_names;
 +   init.num_parents = mux-num_parents;

To be safe, please set

  init.flags = mux-flags;

here

 +
 +   cpumux-reg = mux-mux_reg;
 +   cpumux-shift = mux-mux_shift;
 +   cpumux-mask = (BIT(mux-mux_width) - 1);
 +   cpumux-regmap = regmap;
 +   cpumux-hw.init = init;
 +
 +   clk = clk_register(NULL, cpumux-hw);
 +   if (IS_ERR(clk))
 +   kfree(cpumux);
 +
 +   return clk;
 +}
 +
 +int mtk_clk_register_cpumuxes(struct device_node *node,
 + const struct mtk_composite *clks, int num,
 + struct clk_onecell_data *clk_data)
 +{
 +   int i;
 +   struct clk *clk;
 +   struct regmap *regmap;
 +
 +   if (!clk_data)
 +   return -ENOMEM;
 +
 +   regmap = syscon_node_to_regmap(node);
 +   if (IS_ERR(regmap)) {
 +   pr_err(Cannot find regmap for %s: %ld\n, node-full_name,
 +  PTR_ERR(regmap));
 +   return PTR_ERR(regmap);
 +   }
 +
 +   for (i = 0; i  num; i++) {
 +   const struct mtk_composite *mux = clks[i];
 +
 +   clk = mtk_clk_register_cpumux(mux, regmap);
 +   if (IS_ERR(clk)) {
 +   pr_err(Failed to register clk %s: %ld\n,

[PATCH] clk: mediatek: Initialize clk flags

[Ricky Liang]

The variable init (struct clk_init_data) is allocated on the stack.
We weren't initializing the .flags field, so it contains random junk,
which can cause all kinds of interesting issues when the flags are
parsed by clk_register.

The best solution seems to just pass on the flags passed in to our
clk_register wrappers.

Signed-off-by: Ricky Liang jcli...@chromium.org
---
 drivers/clk/mediatek/clk-pll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 66154ca..72fe307 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -289,6 +289,7 @@ static struct clk *mtk_clk_register_pll(const struct 
mtk_pll_data *data,
init.ops = mtk_pll_ops;
init.parent_names = parent_name;
init.num_parents = 1;
+   init.flags = data-flags;
 
clk = clk_register(NULL, pll-hw);
 
-- 
2.1.2

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

[PATCH] of/fdt: fix allocation size for device node path

[Ricky Liang]

The allocation size of device node path is off by one which drops the
'\0' terminator.

Signed-off-by: Ricky Liang 
---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 3a896c9..98a9e6e 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -172,7 +172,7 @@ static void * unflatten_dt_node(void *blob,
if (!pathp)
return mem;
 
-   allocl = l++;
+   allocl = ++l;
 
/* version 0x10 has a more compact unit name here instead of the full
 * path. we accumulate the full path size using "fpsize", we'll rebuild
-- 
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] of/fdt: fix allocation size for device node path

[Ricky Liang]

The allocation size of device node path is off by one which drops the
'\0' terminator.

Signed-off-by: Ricky Liang jcli...@chromium.org
---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 3a896c9..98a9e6e 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -172,7 +172,7 @@ static void * unflatten_dt_node(void *blob,
if (!pathp)
return mem;
 
-   allocl = l++;
+   allocl = ++l;
 
/* version 0x10 has a more compact unit name here instead of the full
 * path. we accumulate the full path size using fpsize, we'll rebuild
-- 
2.1.2

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