Re: [alsa-devel] [trivial PATCH] treewide: Align function definition open/close braces

[Takashi Iwai]

On Mon, 18 Dec 2017 01:28:44 +0100,
Joe Perches wrote:
> 
> Some functions definitions have either the initial open brace and/or
> the closing brace outside of column 1.
> 
> Move those braces to column 1.
> 
> This allows various function analyzers like gnu complexity to work
> properly for these modified functions.
> 
> Miscellanea:
> 
> o Remove extra trailing ; and blank line from xfs_agf_verify
> 
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
> git diff -w shows no difference other than the above 'Miscellanea'
> 
> (this is against -next, but it applies against Linus' tree
>  with a couple offsets)
> 
>  arch/x86/include/asm/atomic64_32.h   |  2 +-
>  drivers/acpi/custom_method.c |  2 +-
>  drivers/acpi/fan.c   |  2 +-
>  drivers/gpu/drm/amd/display/dc/core/dc.c |  2 +-
>  drivers/media/i2c/msp3400-kthreads.c |  2 +-
>  drivers/message/fusion/mptsas.c  |  2 +-
>  drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c |  2 +-
>  drivers/net/wireless/ath/ath9k/xmit.c|  2 +-
>  drivers/platform/x86/eeepc-laptop.c  |  2 +-
>  drivers/rtc/rtc-ab-b5ze-s3.c |  2 +-
>  drivers/scsi/dpt_i2o.c   |  2 +-
>  drivers/scsi/sym53c8xx_2/sym_glue.c  |  2 +-
>  fs/locks.c   |  2 +-
>  fs/ocfs2/stack_user.c|  2 +-
>  fs/xfs/libxfs/xfs_alloc.c|  5 ++---
>  fs/xfs/xfs_export.c  |  2 +-
>  kernel/audit.c   |  6 +++---
>  kernel/trace/trace_printk.c  |  4 ++--
>  lib/raid6/sse2.c         | 14 +++---
>  sound/soc/fsl/fsl_dma.c  |  2 +-

For sound bits,
  Acked-by: Takashi Iwai <ti...@suse.de>


thanks,

Takashi

Re: [PATCH 20/22] sound: pci: avoid string overflow warnings

[Takashi Iwai]

On Fri, 14 Jul 2017 14:07:12 +0200,
Arnd Bergmann wrote:
> 
> With gcc-7, we get various warnings about a possible string overflow:
> 
> sound/pci/rme9652/hdspm.c: In function 'snd_hdspm_create_alsa_devices':
> sound/pci/rme9652/hdspm.c:2123:17: error: ' MIDIoverMADI' directive writing 
> 13 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
> sound/pci/pcxhr/pcxhr.c: In function 'pcxhr_probe':
> sound/pci/pcxhr/pcxhr.c:1647:28: error: ' [PCM #' directive writing 7 bytes 
> into a region of size between 1 and 32 [-Werror=format-overflow=]
> sound/pci/mixart/mixart.c: In function 'snd_mixart_probe':
> sound/pci/mixart/mixart.c:1353:28: error: ' [PCM #' directive writing 7 bytes 
> into a region of size between 1 and 32 [-Werror=format-overflow=]
>sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);
> ^~
> sound/pci/mixart/mixart.c:1353:28: note: using the range [-2147483648, 
> 2147483647] for directive argument
> sound/pci/mixart/mixart.c:1353:3: note: 'sprintf' output between 10 and 51 
> bytes into a destination of size 32
>sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);
>^~~
> sound/pci/mixart/mixart.c:1354:27: error: ' [PCM #' directive writing 7 bytes 
> into a region of size between 1 and 80 [-Werror=format-overflow=]
>sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
>^~
> sound/pci/mixart/mixart.c:1354:27: note: using the range [-2147483648, 
> 2147483647] for directive argument
> sound/pci/mixart/mixart.c:1354:3: note: 'sprintf' output between 10 and 99 
> bytes into a destination of size 80
> 
> I have checked these all and found that the driver-private
> shortname strings for mixart and pcxhr are longer than necessary,
> and making them shorter will be safe while also making it clear
> that no overflow can happen when they get passed as a substring
> into the card shortname.
> 
> For hdspm, we have a local buffer of the same size as its substring.
> In this case, making the buffer a little longer is safe as the
> functions that take it as an argument all use length checking and
> the strings we pass into it are actually short enough.
> 
> Signed-off-by: Arnd Bergmann 

Thanks for the patch.  I have seen it but ignored, so far, as not sure
which action is the best.  An alternative solution is to use
snprintf() blindly, for example.

For mixart, it's even better to drop mgr->shortname[] and longname[]
assignment.  The shortname is the fixed string, and the longname is
used only at copying to card->longname, so we can create a string
there from the scratch.


Takashi

> ---
>  sound/pci/mixart/mixart.h | 4 ++--
>  sound/pci/pcxhr/pcxhr.h   | 4 ++--
>  sound/pci/rme9652/hdspm.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/pci/mixart/mixart.h b/sound/pci/mixart/mixart.h
> index 426743871540..c8309e327663 100644
> --- a/sound/pci/mixart/mixart.h
> +++ b/sound/pci/mixart/mixart.h
> @@ -75,8 +75,8 @@ struct mixart_mgr {
>   struct mem_area mem[2];
>  
>   /* share the name */
> - char shortname[32]; /* short name of this soundcard */
> - char longname[80];  /* name of this soundcard */
> + char shortname[16]; /* short name of this soundcard */
> + char longname[40];  /* name of this soundcard */
>  
>   /* one and only blocking message or notification may be pending  */
>   u32 pending_event;
> diff --git a/sound/pci/pcxhr/pcxhr.h b/sound/pci/pcxhr/pcxhr.h
> index 9e39e509a3ef..4909a43ce3d9 100644
> --- a/sound/pci/pcxhr/pcxhr.h
> +++ b/sound/pci/pcxhr/pcxhr.h
> @@ -75,8 +75,8 @@ struct pcxhr_mgr {
>   unsigned long port[3];
>  
>   /* share the name */
> - char shortname[32]; /* short name of this soundcard */
> - char longname[96];  /* name of this soundcard */
> + char shortname[16]; /* short name of this soundcard */
> + char longname[40];  /* name of this soundcard */
>  
>   struct pcxhr_rmh *prmh;
>  
> diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
> index 254c3d040118..a1cbf5938a0e 100644
> --- a/sound/pci/rme9652/hdspm.c
> +++ b/sound/pci/rme9652/hdspm.c
> @@ -2061,7 +2061,7 @@ static int snd_hdspm_create_midi(struct snd_card *card,
>struct hdspm *hdspm, int id)
>  {
>   int err;
> - char buf[32];
> + char buf[64];
>  
>   hdspm->midi[id].id = id;
>   hdspm->midi[id].hdspm = hdspm;
> -- 
> 2.9.0
> 
> 

Re: [alsa-devel] [PATCH 04/11] sound/hal2: switch to dma_alloc_attrs

[Takashi Iwai]

On Fri, 16 Jun 2017 10:51:47 +0200,
Christoph Hellwig wrote:
> 
> On Fri, Jun 16, 2017 at 10:49:56AM +0200, Takashi Iwai wrote:
> > On Fri, 16 Jun 2017 09:17:09 +0200,
> > Christoph Hellwig wrote:
> > > 
> > > Use dma_alloc_attrs directly instead of the dma_alloc_noncoherent wrapper.
> > > 
> > > Signed-off-by: Christoph Hellwig <h...@lst.de>
> > 
> > Should I take this one through sound git tree, or would you prefer
> > taking all through another?
> 
> Feel free to take it through the sound tree.

OK, applied now.  Thanks!


Takashi

Re: [alsa-devel] [PATCH 04/11] sound/hal2: switch to dma_alloc_attrs

[Takashi Iwai]

On Fri, 16 Jun 2017 09:17:09 +0200,
Christoph Hellwig wrote:
> 
> Use dma_alloc_attrs directly instead of the dma_alloc_noncoherent wrapper.
> 
> Signed-off-by: Christoph Hellwig <h...@lst.de>

Should I take this one through sound git tree, or would you prefer
taking all through another?

In the latter case,
  Reviewed-by: Takashi Iwai <ti...@suse.de>


thanks,

Takashi

> ---
>  sound/mips/hal2.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
> index 00fc9241d266..17a78a93e885 100644
> --- a/sound/mips/hal2.c
> +++ b/sound/mips/hal2.c
> @@ -461,15 +461,15 @@ static int hal2_alloc_dmabuf(struct hal2_codec *codec)
>   int count = H2_BUF_SIZE / H2_BLOCK_SIZE;
>   int i;
>  
> - codec->buffer = dma_alloc_noncoherent(NULL, H2_BUF_SIZE,
> -   _dma, GFP_KERNEL);
> + codec->buffer = dma_alloc_attrs(NULL, H2_BUF_SIZE, _dma,
> + GFP_KERNEL, DMA_ATTR_NON_CONSISTENT);
>   if (!codec->buffer)
>   return -ENOMEM;
> - desc = dma_alloc_noncoherent(NULL, count * sizeof(struct hal2_desc),
> -  _dma, GFP_KERNEL);
> + desc = dma_alloc_attrs(NULL, count * sizeof(struct hal2_desc),
> +_dma, GFP_KERNEL, DMA_ATTR_NON_CONSISTENT);
>   if (!desc) {
> - dma_free_noncoherent(NULL, H2_BUF_SIZE,
> -  codec->buffer, buffer_dma);
> + dma_free_attrs(NULL, H2_BUF_SIZE, codec->buffer, buffer_dma,
> +DMA_ATTR_NON_CONSISTENT);
>   return -ENOMEM;
>   }
>   codec->buffer_dma = buffer_dma;
> @@ -490,10 +490,10 @@ static int hal2_alloc_dmabuf(struct hal2_codec *codec)
>  
>  static void hal2_free_dmabuf(struct hal2_codec *codec)
>  {
> - dma_free_noncoherent(NULL, codec->desc_count * sizeof(struct hal2_desc),
> -  codec->desc, codec->desc_dma);
> - dma_free_noncoherent(NULL, H2_BUF_SIZE, codec->buffer,
> -  codec->buffer_dma);
> + dma_free_attrs(NULL, codec->desc_count * sizeof(struct hal2_desc),
> +codec->desc, codec->desc_dma, DMA_ATTR_NON_CONSISTENT);
> + dma_free_attrs(NULL, H2_BUF_SIZE, codec->buffer, codec->buffer_dma,
> +DMA_ATTR_NON_CONSISTENT);
>  }
>  
>  static struct snd_pcm_hardware hal2_pcm_hw = {
> -- 
> 2.11.0
> 
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [alsa-devel] [PATCH 2/4] sound: isa: sscape: Use correct format identifier for size_t

[Takashi Iwai]

On Mon, 11 Apr 2016 15:25:52 +0200,
William Breathitt Gray wrote:
> 
> The 'size' member of a struct firmware is passed to snd_printk with a
> respective format string using the %d identifier. The 'size' member is
> of type size_t, but format identifier %d indicates a signed int data
> type. This patch replaces the %d format identifier with the correct %zu
> format identifier for size_t data types.
> 
> Cc: Jaroslav Kysela <pe...@perex.cz>
> Cc: Takashi Iwai <ti...@suse.com>
> Signed-off-by: William Breathitt Gray <vilhelm.g...@gmail.com>

This is basically irrelevant from ISA issue you're trying to solve, so
I'm going to apply this one individually to my tree.


thanks,

Takashi

> ---
>  sound/isa/sscape.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/isa/sscape.c b/sound/isa/sscape.c
> index 7b248cd..fdcfa29 100644
> --- a/sound/isa/sscape.c
> +++ b/sound/isa/sscape.c
> @@ -591,7 +591,7 @@ static int sscape_upload_microcode(struct snd_card *card, 
> int version)
>   }
>   err = upload_dma_data(sscape, init_fw->data, init_fw->size);
>   if (err == 0)
> - snd_printk(KERN_INFO "sscape: MIDI firmware loaded %d KBs\n",
> + snd_printk(KERN_INFO "sscape: MIDI firmware loaded %zu KBs\n",
>   init_fw->size >> 10);
>  
>   release_firmware(init_fw);
> -- 
> 2.7.3
> 
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel BUG at drivers/scsi/scsi_lib.c:1096!

[Takashi Iwai]

On Fri, 04 Dec 2015 18:02:58 +0100,
Jens Axboe wrote:
> 
> On 12/04/2015 09:59 AM, Takashi Iwai wrote:
> > On Wed, 25 Nov 2015 20:01:47 +0100,
> > Hannes Reinecke wrote:
> >>
> >> On 11/25/2015 07:01 PM, Mike Snitzer wrote:
> >>> On Wed, Nov 25 2015 at  4:04am -0500,
> >>> Hannes Reinecke <h...@suse.de> wrote:
> >>>
> >>>> On 11/20/2015 04:28 PM, Ewan Milne wrote:
> >>>>> On Fri, 2015-11-20 at 15:55 +0100, Hannes Reinecke wrote:
> >>>>>> Can't we have a joint effort here?
> >>>>>> I've been spending a _LOT_ of time trying to debug things here, but
> >>>>>> none of the ideas I've come up with have been able to fix anything.
> >>>>>
> >>>>> Yes.  I'm not the one primarily looking at it, and we don't have a
> >>>>> reproducer in-house.  We just have the one dump right now.
> >>>>>
> >>>>>>
> >>>>>> I'm almost tempted to increase the count from scsi_alloc_sgtable()
> >>>>>> by one and be done with ...
> >>>>>>
> >>>>>
> >>>>> That might not fix it if it is a problem with the merge code, though.
> >>>>>
> >>>> And indeed, it doesn't.
> >>>
> >>> How did you arrive at that?  Do you have a reproducer now?
> >>>
> >> Not a reproducer, but several dumps for analysis.
> >>
> >>>> Seems I finally found the culprit.
> >>>>
> >>>> What happens is this:
> >>>> We have two paths, with these seg_boundary_masks:
> >>>>
> >>>> path-1:seg_boundary_mask = 65535,
> >>>> path-2:seg_boundary_mask = 4294967295,
> >>>>
> >>>> consequently the DM request queue has this:
> >>>>
> >>>> md-1:seg_boundary_mask = 65535,
> >>>>
> >>>> What happens now is that a request is being formatted, and sent
> >>>> to path 2. During submission req->nr_phys_segments is formatted
> >>>> with the limits of path 2, arriving at a count of 3.
> >>>> Now the request gets retried on path 1, but as the NOMERGE request
> >>>> flag is set req->nr_phys_segments is never updated.
> >>>> But blk_rq_map_sg() ignores all counters, and just uses the
> >>>> bi_vec directly, resulting in a count of 4 -> boom.
> >>>>
> >>>> So the culprit here is the NOMERGE flag,
> >>>
> >>> NOMERGE is always set in __blk_rq_prep_clone() for cloned requests.
> >>>
> >> Yes.
> >>
> >>>> which is evaluated via
> >>>> ->dm_dispatch_request()
> >>>> ->blk_insert_cloned_request()
> >>>>   ->blk_rq_check_limits()
> >>>
> >>> blk_insert_cloned_request() is the only caller of blk_rq_check_limits();
> >>> anyway after reading your mail I'm still left wondering if your proposed
> >>> patch is correct.
> >>>
> >>>> If the above assessment is correct, the following patch should
> >>>> fix it:
> >>>>
> >>>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>>> index 801ced7..12cccd6 100644
> >>>> --- a/block/blk-core.c
> >>>> +++ b/block/blk-core.c
> >>>> @@ -1928,7 +1928,7 @@ EXPORT_SYMBOL(submit_bio);
> >>>> */
> >>>>int blk_rq_check_limits(struct request_queue *q, struct request *rq)
> >>>>{
> >>>> -   if (!rq_mergeable(rq))
> >>>> +   if (rq->cmd_type != REQ_TYPE_FS)
> >>>>   return 0;
> >>>>
> >>>>   if (blk_rq_sectors(rq) > blk_queue_get_max_sectors(q,
> >>>> rq->cmd_flags)) {
> >>>>
> >>>>
> >>>> Mike? Jens?
> >>>> Can you comment on it?
> >>>
> >>> You're not explaining the actual change in the patch very well; I think
> >>> you're correct but you're leaving the justification as an exercise to
> >>> the reviewer:
> >>>
> >>> blk_rq_check_limits() will call blk_recalc_rq_segments() after the
> >>> !rq_mergeable() check but you're saying for this case in question we
> >>> never get there -- due to the cloned request having NOMERGE set.
> >>&g

Re: [RFC][PATCH] x86: remove vmalloc.h from asm/io.h

[Takashi Iwai]

At Fri, 29 May 2015 19:18:47 +1000,
Stephen Rothwell wrote:
 
 Nothing in asm/io.h uses anything from vmalloc.h, so remove the include
 and fix up the build problems in an allmodconfig (64 bit and 32 bit)
 build.
 
 This may be the place where x86 builds get vmalloc.h implicitly included
 and that tends to hide places where vmalloc() et al are added to files
 but the include of vmalloc.h is forgotten.
 
 Cc: Thomas Gleixner t...@linutronix.de
 Cc: Ingo Molnar mi...@redhat.com
 Cc: H. Peter Anvin h...@zytor.com
 Cc: x...@kernel.org
 Cc: Konrad Rzeszutek Wilk konrad.w...@oracle.com
 Cc: Boris Ostrovsky boris.ostrov...@oracle.com
 Cc: David Vrabel david.vra...@citrix.com
 Cc: Anton Vorontsov an...@enomsg.org
 Cc: Colin Cross ccr...@android.com
 Cc: Kees Cook keesc...@chromium.org
 Cc: Tony Luck tony.l...@intel.com
 Cc: Rafael J. Wysocki r...@rjwysocki.net
 Cc: Len Brown l...@kernel.org
 Cc: Kristen Carlson Accardi kris...@linux.intel.com
 Cc: Viresh Kumar viresh.ku...@linaro.org
 Cc: Vinod Koul vinod.k...@intel.com
 Cc: K. Y. Srinivasan k...@microsoft.com
 Cc: Haiyang Zhang haiya...@microsoft.com
 Cc: Hiral Patel hiral...@cisco.com
 Cc: Suma Ramars sram...@cisco.com
 Cc: Brian Uchino buch...@cisco.com
 Cc: James E.J. Bottomley jbottom...@odin.com
 Cc: Jaroslav Kysela pe...@perex.cz
 Cc: Takashi Iwai ti...@suse.de

For the sound bits,
  Acked-by: Takashi Iwai ti...@suse.de


thanks,

Takashi

 Cc: Andrew Morton a...@linux-foundation.org
 Suggested-by: David Miller da...@davemloft.net
 Signed-off-by: Stephen Rothwell s...@canb.auug.org.au
 
 ---
 
 Based in Linus' tree of today.
 
 There are probably more places that need vmalloc.h included, but this
 passes 64 bit and 32 bit allmodconfig builds, so is a place to start.
 
 Dave Miller suggested that I start this journey.
 
  arch/x86/include/asm/io.h  | 2 --
  arch/x86/kernel/crash.c| 1 +
  arch/x86/kernel/machine_kexec_64.c | 1 +
  arch/x86/mm/pageattr-test.c| 1 +
  arch/x86/mm/pageattr.c | 1 +
  arch/x86/xen/p2m.c | 1 +
  drivers/acpi/apei/erst.c   | 1 +
  drivers/cpufreq/intel_pstate.c | 1 +
  drivers/dma/mic_x100_dma.c | 1 +
  drivers/net/hyperv/netvsc.c| 1 +
  drivers/net/hyperv/rndis_filter.c  | 1 +
  drivers/scsi/fnic/fnic_debugfs.c   | 1 +
  drivers/scsi/fnic/fnic_trace.c | 1 +
  sound/pci/asihpi/hpioctl.c | 1 +
  14 files changed, 13 insertions(+), 2 deletions(-)
 
 diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
 index 34a5b93704d3..5791e7ace9db 100644
 --- a/arch/x86/include/asm/io.h
 +++ b/arch/x86/include/asm/io.h
 @@ -197,8 +197,6 @@ extern void set_iounmap_nonlazy(void);
  
  #include asm-generic/iomap.h
  
 -#include linux/vmalloc.h
 -
  /*
   * Convert a virtual cached pointer to an uncached pointer
   */
 diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
 index c76d3e37c6e1..e068d6683dba 100644
 --- a/arch/x86/kernel/crash.c
 +++ b/arch/x86/kernel/crash.c
 @@ -22,6 +22,7 @@
  #include linux/elfcore.h
  #include linux/module.h
  #include linux/slab.h
 +#include linux/vmalloc.h
  
  #include asm/processor.h
  #include asm/hardirq.h
 diff --git a/arch/x86/kernel/machine_kexec_64.c 
 b/arch/x86/kernel/machine_kexec_64.c
 index 415480d3ea84..11546b462fa6 100644
 --- a/arch/x86/kernel/machine_kexec_64.c
 +++ b/arch/x86/kernel/machine_kexec_64.c
 @@ -17,6 +17,7 @@
  #include linux/ftrace.h
  #include linux/io.h
  #include linux/suspend.h
 +#include linux/vmalloc.h
  
  #include asm/init.h
  #include asm/pgtable.h
 diff --git a/arch/x86/mm/pageattr-test.c b/arch/x86/mm/pageattr-test.c
 index 6629f397b467..8ff686aa7e8c 100644
 --- a/arch/x86/mm/pageattr-test.c
 +++ b/arch/x86/mm/pageattr-test.c
 @@ -9,6 +9,7 @@
  #include linux/random.h
  #include linux/kernel.h
  #include linux/mm.h
 +#include linux/vmalloc.h
  
  #include asm/cacheflush.h
  #include asm/pgtable.h
 diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
 index 89af288ec674..bedfc794b4ba 100644
 --- a/arch/x86/mm/pageattr.c
 +++ b/arch/x86/mm/pageattr.c
 @@ -14,6 +14,7 @@
  #include linux/percpu.h
  #include linux/gfp.h
  #include linux/pci.h
 +#include linux/vmalloc.h
  
  #include asm/e820.h
  #include asm/processor.h
 diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
 index b47124d4cd67..8b7f18e200aa 100644
 --- a/arch/x86/xen/p2m.c
 +++ b/arch/x86/xen/p2m.c
 @@ -67,6 +67,7 @@
  #include linux/seq_file.h
  #include linux/bootmem.h
  #include linux/slab.h
 +#include linux/vmalloc.h
  
  #include asm/cache.h
  #include asm/setup.h
 diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
 index ed65e9c4b5b0..3670bbab57a3 100644
 --- a/drivers/acpi/apei/erst.c
 +++ b/drivers/acpi/apei/erst.c
 @@ -35,6 +35,7 @@
  #include linux/nmi.h
  #include linux/hardirq.h
  #include linux/pstore.h
 +#include linux/vmalloc.h
  #include acpi/apei.h
  
  #include apei-internal.h
 diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Takashi Iwai]

At Sun, 17 Aug 2014 20:21:38 +0200,
Oleg Nesterov wrote:
 
 On 08/17, Luis R. Rodriguez wrote:
 
  In the last iteration that I have stress tested for corner cases I just
  get_task_struct() on the init and then put_task_struct() at the exit, is 
  that
  fine too or are there reasons to prefer the module stuff?
 
 I am fine either way.
 
 I like the Takashi's idea because if sys_delete_module() is called before
 initfn() completes it will return -EBUSY and not hang in TASK_UNINTERRUPTIBLE
 state. But this is not necessarily good, so I leave this to you and Takashi.

Another merit of fiddling with module count is that the thread object
isn't referred in other than module_init.  That is, we'd need only
module_init() implementation like below (thanks to Oleg's advice):

#define module_long_probe_init(initfn)  \
static int _long_probe_##initfn(void *arg)  \
{   \
module_put_and_exit(initfn());  \
return 0;   \
}   \
static int __init __long_probe_##initfn(void)   \
{   \
struct task_struct *__init_thread = \
kthread_create(_long_probe_##initfn,\
   NULL, #initfn);  \
if (IS_ERR(__init_thread))  \
return PTR_ERR(__init_thread);  \
__module_get(THIS_MODULE);  \
wake_up_process(__init_thread); \
return 0;   \
}   \
module_init(__long_probe_##initfn)

... and module_exit() remains identical as the normal version.

But, it's really a small difference, and I don't mind much which way
to take, too.

  +/*
  + * Linux device drivers must strive to handle driver initialization
  + * within less than 30 seconds,
 
 Well, perhaps the comment should name the reason ;)
 
  if device probing takes longer
  + * for whatever reason asynchronous probing of devices / loading
  + * firmware should be used. If a driver takes longer than 30 second
  + * on the initialization path
 
 Or if the initialization code can't handle the errors properly (say,
 mptsas can't handle the errors caused by SIGKILL).
 
  + * Drivers that use this helper should be considered broken and in need
  + * of some serious love.
  + */
 
 Yes.
 
  +#define module_long_probe_init(initfn) \
  +   static struct task_struct *__init_thread;   \
  +   static int _long_probe_##initfn(void *arg)  \
  +   {   \
  +   return initfn();\
  +   }   \
  +   static inline __init int __long_probe_##initfn(void)\
  +   {   \
  +   __init_thread = kthread_create(_long_probe_##initfn,\
  +  NULL,\
  +  #initfn);\
  +   if (IS_ERR(__init_thread))  \
  +   return PTR_ERR(__init_thread);  \
  +   /*  \
  +* callback won't check kthread_should_stop()   \
  +* before bailing, so we need to protect it \
  +* before running it.   \
  +*/ \
  +   get_task_struct(__init_thread); \
  +   wake_up_process(__init_thread); \
  +   return 0;   \
  +   }   \
  +   module_init(__long_probe_##initfn);
  +
  +/* To be used by modules that require module_long_probe_init() */
  +#define module_long_probe_exit(exitfn) \
  +   static inline void __long_probe_##exitfn(void)  \
  +   {   \
  +   int err;\
  +   /*  \
  +* exitfn() will not be run if the driver's \
  +* real probe which is run on the kthread   \
  +* failed for whatever reason, this will\
  +* wait for it to end.  \
  +*/ \
  +   err = kthread_stop(__init_thread);  \
  +   if (!err)

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Takashi Iwai]

At Mon, 18 Aug 2014 14:22:17 +0200,
Oleg Nesterov wrote:
 
 On 08/18, Takashi Iwai wrote:
 
  #define module_long_probe_init(initfn)  \
  static int _long_probe_##initfn(void *arg)  \
  {   \
  module_put_and_exit(initfn());  \
  return 0;   \
  }   \
  static int __init __long_probe_##initfn(void)   \
  {   \
  struct task_struct *__init_thread = \
  kthread_create(_long_probe_##initfn,\
 NULL, #initfn);  \
  if (IS_ERR(__init_thread))  \
  return PTR_ERR(__init_thread);  \
  __module_get(THIS_MODULE);  \
  wake_up_process(__init_thread); \
  return 0;   \
  }   \
  module_init(__long_probe_##initfn)
 
  ... and module_exit() remains identical as the normal version.
 
 Aaaah. This is not true, module_exit() should not call exitfn() if initfn()
 fails... So _long_probe_##initfn() needs to save the error code which should
 be checked by module_exit().

Oh, right.  So we need a reference in the module exit path in anyway,
and Luis' version might be shorter in the end.


thanks,

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

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Takashi Iwai]

At Sat, 16 Aug 2014 04:50:07 +0200,
Luis R. Rodriguez wrote:
 
 On Fri, Aug 15, 2014 at 04:39:02PM +0200, Oleg Nesterov wrote:
  On 08/15, Luis R. Rodriguez wrote:
  
   On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
On 08/12, Luis R. Rodriguez wrote:

 +/* To be used by modules which can take over 30 seconds at probe */
   
Probably the comment should explain that this hack should only be
used if the driver is buggy and is wating for real fix.
   
 +#define module_long_probe_init(initfn)   
 \
 + static struct task_struct *__init_thread;   \
 + static int _long_probe_##initfn(void *arg)  \
 + {   \
 + return initfn();\
 + }   \
 + static inline __init int __long_probe_##initfn(void)\
 + {   \
 + __init_thread = kthread_run(_long_probe_##initfn,\
 + NULL,   \
 + #initfn);   \
 + if (IS_ERR(__init_thread))  \
 + return PTR_ERR(__init_thread);  \
 + return 0;   \
 + }   \
 + module_init(__long_probe_##initfn);
 +/* To be used by modules that require module_long_probe_init() */
 +#define module_long_probe_exit(exitfn)   
 \
 + static inline void __long_probe_##exitfn(void)  \
 + {   \
 + exitfn();   \
 + if (__init_thread)  \
 + kthread_stop(__init_thread);\
 + }   \
   
exitfn() should be called after kthread_stop(), and only if initfn()
returns 0. So it should probably do
   
int err = kthread_stop(__init_thread);
if (!err)
exitfn();
  
   Thanks! With the check for __init_thread as well as it can be
   ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
   reason).
  
  Do you mean __long_probe_##exitfn() should also check 
  ERR_PTR(__init_thread)?
  I don't think so. If kthread_run() above fails, module_init() should return
  the error (it does), so module_exit() won't be called.
 
 Good point.
 
But there is an additional complication, you can't use __init_thread
without get_task_struct(),
  
   Can you elaborate why ? kthread_stop() uses get_task_struct(),
  
  This is too late. This task_struct can be already freed/reused. See below.
  
   wake_up_process() and finally put_task_struct(), and we're the
   only user of this thread. Also kthread_run() ensures wake_up_process()
   gets called on startup, so not sure where the race would be provided
   all users here and with the respective helpers on buggy drivers.
  
so  __long_probe_##initfn() can't use
kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
  
   I fail to see why we'd need to add get_task_struct() on
   module_long_probe_init(), can you clarify?
  
  kthread_stop(kthread_run(callback)) is only safe if callback() can not exit
  on its own, without checking kthread_should_stop(). And btw that is why
  kthread_stop() does get_task_struct()).
  
  If callback() can exit (if it calls do_exit() or simply returns), then 
  nothing
  protects this task_struct, it will be freed.
 
 OK thanks, yeah I see the issue now, and I was able to create a null
 pointer dereference by simply calling schedule() quite a bit, will
 roll in the required fixes, but come to think of it if there are
 other uses (I haven't SmPLd grep'd for grammar uses yet) perhaps
 generic helpers would be good? kthread_run_alloc() kthread_run_free().

How about just increasing/decreasing the module count for blocking the
exit call?  For example:

#define module_long_probe_init(initfn)  \
static int _long_probe_##initfn(void *arg)  \
{   \
int ret = initfn(); \
module_put(THIS_MODULE);\
return ret; \
}   \
static inline __init int __long_probe_##initfn(void)\
{   \
struct task_struct *__init_thread;  \

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Takashi Iwai]

At Sun, 10 Aug 2014 16:58:02 +0200,
Luis R. Rodriguez wrote:
 
 On Sun, Aug 10, 2014 at 08:43:31PM +0800, Greg KH wrote:
  On Sat, Aug 09, 2014 at 06:41:19PM +0200, Luis R. Rodriguez wrote:
   On Wed, Jul 30, 2014 at 03:11:07PM -0700, David Miller wrote:
From: Luis R. Rodriguez mcg...@do-not-panic.com
Date: Mon, 28 Jul 2014 11:28:28 -0700

 Tetsuo bisected and found that commit 786235ee kthread: make
 kthread_create() killable modified kthread_create() to bail as
 soon as SIGKILL is received. This is causing some issues with
 some drivers and at times boot. Joseph then found that failures
 occur as the systemd-udevd process sends SIGKILL to modprobe if
 probe on a driver takes over 30 seconds. When this happens probe
 will fail on any driver, its why booting on some system will fail
 if the driver happens to be a storage related driver. Some folks
 have suggested fixing this by modifying kthread_create() to not
 leave upon SIGKILL [3], upon review Oleg rejected this change and
 the discussion was punted out to systemd to see if the default
 timeout could be increased from 30 seconds to 120. The opinion of
 the systemd maintainers is that the driver's behavior should
 be fixed [4]. Linus seems to agree [5], however more recently even
 networking drivers have been reported to fail on probe since just
 writing the firmware to a device and kicking it can take easy over
 60 seconds [6]. Benjamim was able to trace the issues recently
 reported on cxgb4 down to the same systemd-udevd 30 second timeout 
 [6].
 
 This is an alternative solution which enables drivers that are
 known to take long to use deferred probe workqueue. This avoids
 the 30 second timeout and lets us annotate drivers with long
 init sequences.
 
 As drivers determine a component is not yet available and needs
 to defer probe you'll be notified this happen upon init for each
 device but now with a message such as:
 
 pci :03:00.0: Driver cxgb4 requests probe deferral on init
 
 You should see one of these per struct device probed.

It seems we're still discussing this.

I think I understand all of the underlying issues, and what I'll say
is that perhaps we should use what Greg KH requested but via a helper
that is easy to grep for.

I don't care if it's something like module_long_probe_init() and
module_long_probe_exit(), but it just needs to be some properly
named interface which does the whole kthread or whatever bit.
   
   I've tested the alternative kthread_run() proposal but unfortunately it
   does not help resolve the issue, the timeout is still hit and a SIGKILL
   still kills the driver probe. Please let me know how you'd all like us
   to proceed, these defer probe patches do help cure the issue though.
  
  Why doesn't it work?  Doesn't modprobe come right back and the init
  sequence still takes a while to run?  What exactly fails?
 
 systemd uses kmod kmod_module_probe_insert_module(), what I see is that using
 kthread_run() as a work around still causes probe to fail on a driver that
 otherwise would work fine if all you do is sprinkle ssleep(33) (seconds) on 
 the
 init sequence. To see the issue you can test this on any of your network
 drivers that get loaded automatically, I did my testing with iwlwifi by
 purposely breaking it and then using the work around. It seems the probe
 somehow still gets killed as before, I haven't debugged this further.
 
 For example by breaking and fixing iwlwifi this yields:
 
 ergon:~ # journalctl -b -0 -u systemd-udevd 
 
 -- Logs begin at Mon 2014-08-04 21:55:28 EDT, end at Sun 2014-08-10 10:50:14 
 EDT. --
 Aug 10 10:48:49 ergon systemd-udevd[257]: specified group 'input' unknown
 Aug 10 10:48:55 ergon mtp-probe[493]: checking bus 3, device 2: 
 /sys/devices/pci:00/:00:14.0/usb3/3-7
 Aug 10 10:48:55 ergon mtp-probe[492]: checking bus 3, device 4: 
 /sys/devices/pci:00/:00:14.0/usb3/3-12
 Aug 10 10:49:24 ergon systemd-udevd[465]: seq 1755 
 '/devices/pci:00/:00:1c.1/:03:00.0' killed
 
 ergon:~ # dmesg | grep iwlwifi
 [   10.315538] iwlwifi Going to sleep for 33 seconds
 [   43.323958] iwlwifi Done sleeping
 [   43.324400] iwlwifi :03:00.0: irq 50 for MSI/MSI-X
 [   43.324534] iwlwifi :03:00.0: Error allocating IRQ 50
 [   43.326951] iwlwifi: probe of :03:00.0 failed with error -4
 
 The patch used:
 
 diff --git a/drivers/net/wireless/iwlwifi/mvm/ops.c 
 b/drivers/net/wireless/iwlwifi/mvm/ops.c
 index 610dbcb..65e0ab2 100644
 --- a/drivers/net/wireless/iwlwifi/mvm/ops.c
 +++ b/drivers/net/wireless/iwlwifi/mvm/ops.c
 @@ -63,6 +63,7 @@
  #include linux/module.h
  #include linux/vmalloc.h
  #include net/mac80211.h
 +#include linux/kthread.h
  
  #include iwl-notif-wait.h
  #include iwl-trans.h
 @@ -111,7 +112,7 @@ MODULE_PARM_DESC(power_scheme,
  /*
   * module init and 

Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

[Takashi Iwai]

At Mon, 28 Jul 2014 07:34:25 -0700,
Luis R. Rodriguez wrote:
 
 From: Luis R. Rodriguez mcg...@suse.com
 
 Tetsuo bisected and found that commit 786235ee kthread: make
 kthread_create() killable modified kthread_create() to bail as
 soon as SIGKILL is received. This is causing some issues with
 some drivers and at times boot. Joseph then found that failures
 occur as the systemd-udevd process sends SIGKILL to modprobe if
 probe on a driver takes over 30 seconds. When this happens probe
 will fail on any driver, its why booting on some system will fail
 if the driver happens to be a storage related driver. Some folks
 have suggested fixing this by modifying kthread_create() to not
 leave upon SIGKILL [3], upon review Oleg rejected this change and
 the discussion was punted out to systemd to see if the default
 timeout could be increased from 30 seconds to 120. The opinion of
 the systemd maintainers is that the driver's behavior should
 be fixed [4]. Linus seems to agree [5], however more recently even
 networking drivers have been reported to fail on probe since just
 writing the firmware to a device and kicking it can take easy over
 60 seconds [6]. Benjamim was able to trace the issues recently
 reported on cxgb4 down to the same systemd-udevd 30 second timeout [6].
 
 This is an alternative solution which enables drivers that are
 known to take long to use deferred probe workqueue. This avoids
 the 30 second timeout and lets us annotate drivers with long
 init sequences.
 
 As drivers determine a component is not yet available and needs
 to defer probe you'll be notified this happen upon init for each
 device but now with a message such as:
 
 pci :03:00.0: Driver cxgb4 requests probe deferral on init
 
 You should see one of these per struct device probed.
 
 [0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
 [1] https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
 [2] http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html
 [3] http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
 [4] http://article.gmane.org/gmane.comp.sysutils.systemd.devel/17860
 [5] http://article.gmane.org/gmane.linux.kernel/1671333
 [6] https://bugzilla.novell.com/show_bug.cgi?id=877622
 
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Cc: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
 Cc: Joseph Salisbury joseph.salisb...@canonical.com
 Cc: Kay Sievers k...@vrfy.org
 Cc: One Thousand Gnomes gno...@lxorguk.ukuu.org.uk
 Cc: Tim Gardner tim.gard...@canonical.com
 Cc: Pierre Fersing pierre-fers...@pierref.org
 Cc: Andrew Morton a...@linux-foundation.org
 Cc: Oleg Nesterov o...@redhat.com
 Cc: Benjamin Poirier bpoir...@suse.de
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Cc: Nagalakshmi Nandigama nagalakshmi.nandig...@avagotech.com
 Cc: Praveen Krishnamoorthy praveen.krishnamoor...@avagotech.com
 Cc: Sreekanth Reddy sreekanth.re...@avagotech.com
 Cc: Abhijit Mahajan abhijit.maha...@avagotech.com
 Cc: Hariprasad S haripra...@chelsio.com
 Cc: Santosh Rastapur sant...@chelsio.com
 Cc: mpt-fusionlinux@avagotech.com
 Cc: linux-scsi@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org
 Cc: net...@vger.kernel.org
 Signed-off-by: Luis R. Rodriguez mcg...@suse.com
 ---
  drivers/base/dd.c  | 15 ++-
  include/linux/device.h | 12 
  2 files changed, 26 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/base/dd.c b/drivers/base/dd.c
 index e4ffbcf..7a271dc 100644
 --- a/drivers/base/dd.c
 +++ b/drivers/base/dd.c
 @@ -374,6 +374,19 @@ void wait_for_device_probe(void)
  }
  EXPORT_SYMBOL_GPL(wait_for_device_probe);
  
 +static int __driver_probe_device(struct device_driver *drv, struct device 
 *dev)
 +{
 + if (drv-delay_probe  !dev-init_delayed_probe) {
 + dev_info(dev, Driver %s requests probe deferral on init\n,
 +  drv-name);
 + dev-init_delayed_probe = true;
 + driver_deferred_probe_add(dev);
 + return -EPROBE_DEFER;
 + }
 +
 + return really_probe(dev, drv);
 +}
 +
  /**
   * driver_probe_device - attempt to bind device  driver together
   * @drv: driver to bind a device to
 @@ -396,7 +409,7 @@ int driver_probe_device(struct device_driver *drv, struct 
 device *dev)
drv-bus-name, __func__, dev_name(dev), drv-name);
  
   pm_runtime_barrier(dev);
 - ret = really_probe(dev, drv);
 + ret = __driver_probe_device(drv, dev);
   pm_request_idle(dev);
  
   return ret;
 diff --git a/include/linux/device.h b/include/linux/device.h
 index af424ac..11da1b7 100644
 --- a/include/linux/device.h
 +++ b/include/linux/device.h
 @@ -200,6 +200,12 @@ extern struct klist *bus_get_device_klist(struct 
 bus_type *bus);
   * @owner:   The module owner.
   * @mod_name:Used for built-in modules.
   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
 + * @delay_probe: this driver is requesting a deferred probe since
 + *