Re: [RFC PATCH 1/7] usb: typec: Generalize mux mode names

2018-05-08 Thread Mats Karrman 
Hi,

On 05/08/2018 04:25 PM, Heikki Krogerus wrote:

> Hi,
>
> On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote:
 Even so, when the mux driver "set" function is called, it will just get the
 mode argument but since the mode (TYPEC_STATE_...) is overlapping for 
 different
 AMs if I understand your proposal correctly, the mux also needs to know 
 what AM
 is active.
 Does this imply that the mux set function signature need to change?
>>> My plan was actually to propose we get rid of the current mux handling
>>> (just leave the orientation switch) in favour of the notifications I'm
>>> introducing with the type-c bus for the alternate modes. The current
>>> mux handling is definitely not enough, and does not work in every
>>> scenario, like also you pointed out.
>> So, the mux need to subscribe to each svid:mode pair it is interested in 
>> using 
>> typec_altmode_register_notifier() and then use those callbacks to switch the 
>> correct
>> signals to the connector. And a driver for an off-the-shelf mux device could 
>> have
>> the translation between svid:mode pairs and mux device specific control 
>> specified by
>> of/acpi properties. Right?
> Yes. That is the plan. Would it work for you?

I think so. I'll give it a go. When about do you think you'll post the next 
version
of your RFC? Or do you have an updated series available somewhere public?

BR // Mats

> Thanks,

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 2/2] usb: misc: xapea00x: perform platform initialization of TPM

2018-05-08 Thread James Bottomley 
On Tue, 2018-05-08 at 10:29 -0500, David R. Bild wrote:
> On Tue, May 8, 2018 at 10:25 AM, James Bottomley
>  wrote:
> > 
> > > On Fri, May 04, 2018 at 02:56:25PM -0500, David R. Bild wrote:
> > 
> > [...]
> > > > In particular, it sets the credentials for the platform
> > > > hierarchy.
> > > > The platform hierarchy is essentially the "root" account of the
> > > > TPM, so it's critical that those credentials be set before the
> > > > TPM
> > > > is exposed to user-space.  (The platform credentials aren't
> > > > persisted in the TPM and must be set by the platform on every
> > > > boot.)  If the driver registers the TPM before doing
> > > > initialization, there's a chance that something else could
> > > > access
> > > > the TPM before the platform credentials get set.
> > 
> > I don't see any reason to set an unreachable password for the
> > platform
> > hierarchy if the UEFI didn't.  If the desire is to disable the
> > platform
> > hierarchy, then it should be disabled, not have a random password
> > set.
> 
> "Set random password and throw away the key" was my way of disabling
> the platform hierarchy.  Is there a better way of doing that?

Well, yes, use TPM2_HierarchyControl to set phEnable to CLEAR.

> > I'd also say this is probably the job of early boot based on
> > policy.
> 
> Agreed.  And since this card has no "early boot", the driver/kernel
> need to do it.

Early boot means userspace. for a hot pluggable device, this would
probably be something in udev if you follow the no-daemon model and the
daemon could do it if you do follow the daemon model.

James

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

Re: linux-next: Tree for May 8 (usb: typec...)

2018-05-08 Thread Randy Dunlap 
On 05/07/2018 10:22 PM, Stephen Rothwell wrote:
> Hi all,
> 
> Changes since 20180507:
> 

on x86_64:

WARNING: unmet direct dependencies detected for TYPEC_TCPCI
  Depends on [n]: STAGING [=y] && TYPEC_TCPM [=y] && I2C [=n]
  Selected by [y]:
  - TYPEC_RT1711H [=y] && STAGING [=y] && TYPEC_TCPM [=y]

=>

../drivers/staging/typec/tcpci.c:593:1: warning: data definition has no type or 
storage class [enabled by default]
 module_i2c_driver(tcpci_i2c_driver);
 ^
../drivers/staging/typec/tcpci.c:593:1: error: type defaults to 'int' in 
declaration of 'module_i2c_driver' [-Werror=implicit-int]
../drivers/staging/typec/tcpci.c:593:1: warning: parameter names (without 
types) in function declaration [enabled by default]
../drivers/staging/typec/tcpci.c:584:26: warning: 'tcpci_i2c_driver' defined 
but not used [-Wunused-variable]
 static struct i2c_driver tcpci_i2c_driver = {
  ^


Full randconfig file is attached.

-- 
~Randy
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86_64 4.17.0-rc4 Kernel Configuration
#
CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86=y
CONFIG_INSTRUCTION_DECODER=y
CONFIG_OUTPUT_FORMAT="elf64-x86-64"
CONFIG_ARCH_DEFCONFIG="arch/x86/configs/x86_64_defconfig"
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_STACKTRACE_SUPPORT=y
CONFIG_MMU=y
CONFIG_ARCH_MMAP_RND_BITS_MIN=28
CONFIG_ARCH_MMAP_RND_BITS_MAX=32
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MIN=8
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MAX=16
CONFIG_NEED_DMA_MAP_STATE=y
CONFIG_NEED_SG_DMA_LENGTH=y
CONFIG_GENERIC_ISA_DMA=y
CONFIG_GENERIC_BUG=y
CONFIG_GENERIC_BUG_RELATIVE_POINTERS=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_ARCH_MAY_HAVE_PC_FDC=y
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_ARCH_HAS_CPU_RELAX=y
CONFIG_ARCH_HAS_CACHE_LINE_SIZE=y
CONFIG_ARCH_HAS_FILTER_PGPROT=y
CONFIG_HAVE_SETUP_PER_CPU_AREA=y
CONFIG_NEED_PER_CPU_EMBED_FIRST_CHUNK=y
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
CONFIG_ARCH_SUSPEND_POSSIBLE=y
CONFIG_ARCH_WANT_HUGE_PMD_SHARE=y
CONFIG_ARCH_WANT_GENERAL_HUGETLB=y
CONFIG_ZONE_DMA32=y
CONFIG_AUDIT_ARCH=y
CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING=y
CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y
CONFIG_HAVE_INTEL_TXT=y
CONFIG_X86_64_SMP=y
CONFIG_ARCH_SUPPORTS_UPROBES=y
CONFIG_FIX_EARLYCON_MEM=y
CONFIG_PGTABLE_LEVELS=4
CONFIG_CONSTRUCTORS=y
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_EXTABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y

#
# General setup
#
CONFIG_INIT_ENV_ARG_LIMIT=32
CONFIG_CROSS_COMPILE=""
CONFIG_COMPILE_TEST=y
CONFIG_LOCALVERSION=""
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
# CONFIG_KERNEL_GZIP is not set
# CONFIG_KERNEL_BZIP2 is not set
# CONFIG_KERNEL_LZMA is not set
# CONFIG_KERNEL_XZ is not set
CONFIG_KERNEL_LZO=y
# CONFIG_KERNEL_LZ4 is not set
CONFIG_DEFAULT_HOSTNAME="(none)"
# CONFIG_SYSVIPC is not set
CONFIG_POSIX_MQUEUE=y
CONFIG_CROSS_MEMORY_ATTACH=y
# CONFIG_USELIB is not set
CONFIG_AUDIT=y
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
CONFIG_AUDITSYSCALL=y
CONFIG_AUDIT_WATCH=y
CONFIG_AUDIT_TREE=y

#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=y
CONFIG_GENERIC_PENDING_IRQ=y
CONFIG_GENERIC_IRQ_MIGRATION=y
CONFIG_GENERIC_IRQ_CHIP=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_DOMAIN_HIERARCHY=y
CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS=y
CONFIG_GENERIC_MSI_IRQ=y
CONFIG_GENERIC_MSI_IRQ_DOMAIN=y
CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y
CONFIG_GENERIC_IRQ_RESERVATION_MODE=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
# CONFIG_GENERIC_IRQ_DEBUGFS is not set
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_ARCH_CLOCKSOURCE_DATA=y
CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE=y
CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
CONFIG_GENERIC_CMOS_UPDATE=y

#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
# CONFIG_NO_HZ_IDLE is not set
CONFIG_NO_HZ_FULL=y
# CONFIG_NO_HZ is not set
CONFIG_HIGH_RES_TIMERS=y

#
# CPU/Task time and stats accounting
#
CONFIG_VIRT_CPU_ACCOUNTING=y
CONFIG_VIRT_CPU_ACCOUNTING_GEN=y
# CONFIG_IRQ_TIME_ACCOUNTING is not set
CONFIG_CPU_ISOLATION=y

#
# RCU Subsystem
#
CONFIG_TREE_RCU=y
CONFIG_RCU_EXPERT=y
CONFIG_SRCU=y
CONFIG_TREE_SRCU=y
CONFIG_TASKS_RCU=y
CONFIG_RCU_STALL_COMMON=y
CONFIG_RCU_NEED_SEGCBLIST=y
CONFIG_CONTEXT_TRACKING=y
# CONFIG_CONTEXT_TRACKING_FORCE is not set
CONFIG_RCU_FANOUT=64
CONFIG_RCU_FANOUT_LEAF=16
# CONFIG_RCU_FAST_NO_HZ is not set
CONFIG_RCU_NOCB_CPU=y
# CONFIG_IKCONFIG is not set
CONFIG_HAVE_UNSTABLE_SCHED_CLOCK=y
CONFIG_ARCH_SUPPORTS_NUMA_BALANCING=y
CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH=y
CONFIG_ARCH_SUPPORTS_INT128=y
# CONFIG_CGROUPS is not set
# CONFIG_SCHED_AUTOGROUP is not set
# CONFIG_SYSFS_DEPRECATED is not set
# CONFIG_RELAY is not set
# CONFIG_BLK_DEV_INITRD is not set
# CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE is not set
CONFIG_CC_OPTIMIZE_FOR_SIZE=y

Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

2018-05-08 Thread Doug Anderson 
Hi,

On Tue, May 8, 2018 at 12:43 AM, wlf  wrote:
> Dear Doug,
>
> 在 2018年05月08日 13:11, Doug Anderson 写道:
>>
>> Hi,
>>
>> On Mon, May 7, 2018 at 8:07 PM, William Wu 
>> wrote:
>>>
>>> +static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
>>> +   struct dwc2_qh *qh,
>>> +   struct dwc2_host_chan *chan)
>>> +{
>>> +   if (!hsotg->unaligned_cache)
>>> +   return -ENOMEM;
>>> +
>>> +   if (!qh->dw_align_buf) {
>>> +   qh->dw_align_buf =
>>> kmem_cache_alloc(hsotg->unaligned_cache,
>>> +   GFP_ATOMIC |
>>> GFP_DMA);
>>> +   if (!qh->dw_align_buf)
>>> +   return -ENOMEM;
>>> +
>>> +   qh->dw_align_buf_size = min_t(u32, chan->max_packet,
>>> +
>>> DWC2_KMEM_UNALIGNED_BUF_SIZE);
>>
>> Rather than using min_t, wouldn't it be better to return -ENOMEM if
>> "max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE?  As it is, you might
>> allocate less space than you need, right?  That seems like it would be
>> bad (even though this is probably impossible).
>
> Yes, good idea! So is it good to fix it like this?
>
> if (!qh->dw_align_buf || chan->max_packet >
> DWC2_KMEM_UNALIGNED_BUF_SIZE)
> return -ENOMEM;
>
> qh->dw_align_buf_size = chan->max_packet;

Won't that orphan memory in the case that the allocation is OK but the
size is wrong?

I would have put it all the way at the top:

if (!hsotg->unaligned_cache ||
chan->max_packet > DWC2_KMEM_UNALIGNED_BUF_SIZE)
  return -ENOMEM;


It also feels like you should get rid of "qh->dw_align_buf_size".  The
buffer is always DWC2_KMEM_UNALIGNED_BUF_SIZE.

...if you need to keep track of how many bytes you mapped with
dma_map_single() and somehow can't get back to "chan", rename this to
qh->dw_align_buf_mapped_bytes or something.  I haven't followed
through all the code, but I _think_ you'd want to make sure to set
qh->dw_align_buf_mapped_bytes every time through
dwc2_alloc_split_dma_aligned_buf(), even if dw_align_buf was already
allocated.  Specifically, my worry is in the case where you enter
dwc2_alloc_split_dma_aligned_buf() and qh->dw_align_buf is non-NULL.
Could "max_packet" be different in this case compared to what it was
when dw_align_buf was first allocated?



>>> @@ -2797,6 +2837,32 @@ static int dwc2_assign_and_init_hc(struct
>>> dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>>>  /* Set the transfer attributes */
>>>  dwc2_hc_init_xfer(hsotg, chan, qtd);
>>>
>>> +   /* For non-dword aligned buffers */
>>> +   if (hsotg->params.host_dma && qh->do_split &&
>>> +   chan->ep_is_in && (chan->xfer_dma & 0x3)) {
>>> +   dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
>>> +   if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
>>> +   dev_err(hsotg->dev,
>>> +   "Failed to allocate memory to handle
>>> non-aligned buffer\n");
>>> +   /* Add channel back to free list */
>>> +   chan->align_buf = 0;
>>> +   chan->multi_count = 0;
>>> +   list_add_tail(>hc_list_entry,
>>> + >free_hc_list);
>>> +   qtd->in_process = 0;
>>> +   qh->channel = NULL;
>>> +   return -ENOMEM;
>>> +   }
>>> +   } else {
>>> +   /*
>>> +* We assume that DMA is always aligned in non-split
>>> +* case or split out case. Warn if not.
>>> +*/
>>> +   WARN_ON_ONCE(hsotg->params.host_dma &&
>>> +(chan->xfer_dma & 0x3));
>>> +   chan->align_buf = 0;
>>> +   }
>>> +
>>>  if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
>>>  chan->ep_type == USB_ENDPOINT_XFER_ISOC)
>>>  /*
>>> @@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>>>  hsotg->params.dma_desc_enable = false;
>>>  hsotg->params.dma_desc_fs_enable = false;
>>>  }
>>> +   } else if (hsotg->params.host_dma) {
>>
>> Are you sure this is "else if"?  Can't you have descriptor DMA enabled
>> in the controller and still need to do a normal DMA transfer if you
>> plug in a hub?  Seems like this should be just "if".
>
> Sorry, I don't understand the case "have descriptor DMA enabled in the
> controller and still need to do a normal DMA transfer". But maybe it still
> has another problem if just use "if" here, because it will create kmem
> caches for Slave mode which actually doesn't need aligned DMA buf.

So right now your code looks like:

if (hsotg->params.dma_desc_enable ||
hsotg->params.dma_desc_fs_enable) {
...
...
} else if (hsotg->params.host_dma) {
   ...

Re: [PATCH v3 2/2] usb: misc: xapea00x: perform platform initialization of TPM

2018-05-08 Thread David R. Bild 
On Tue, May 8, 2018 at 10:25 AM, James Bottomley
 wrote:
>
> > On Fri, May 04, 2018 at 02:56:25PM -0500, David R. Bild wrote:
> [...]
> > > In particular, it sets the credentials for the platform hierarchy.
> > > The platform hierarchy is essentially the "root" account of the
> > > TPM, so it's critical that those credentials be set before the TPM
> > > is exposed to user-space.  (The platform credentials aren't
> > > persisted in the TPM and must be set by the platform on every
> > > boot.)  If the driver registers the TPM before doing
> > > initialization, there's a chance that something else could access
> > > the TPM before the platform credentials get set.
>
> I don't see any reason to set an unreachable password for the platform
> hierarchy if the UEFI didn't.  If the desire is to disable the platform
> hierarchy, then it should be disabled, not have a random password set.

"Set random password and throw away the key" was my way of disabling
the platform hierarchy.  Is there a better way of doing that?

> I'd also say this is probably the job of early boot based on policy.

Agreed.  And since this card has no "early boot", the driver/kernel
need to do it.

Best,
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 2/2] usb: misc: xapea00x: perform platform initialization of TPM

2018-05-08 Thread James Bottomley 
On Tue, 2018-05-08 at 13:55 +0300, Jarkko Sakkinen wrote:
> On Fri, May 04, 2018 at 02:56:25PM -0500, David R. Bild wrote:
[...]
> > In particular, it sets the credentials for the platform hierarchy.
> > The platform hierarchy is essentially the "root" account of the
> > TPM, so it's critical that those credentials be set before the TPM
> > is exposed to user-space.  (The platform credentials aren't
> > persisted in the TPM and must be set by the platform on every
> > boot.)  If the driver registers the TPM before doing
> > initialization, there's a chance that something else could access
> > the TPM before the platform credentials get set.
> 
> Maybe. Not sure yet where to draw the line eg should TSS2 daemon to
> do it for example.
> 
> James? Philip?

I don't see any reason to set an unreachable password for the platform
hierarchy if the UEFI didn't.  If the desire is to disable the platform
hierarchy, then it should be disabled, not have a random password set. 
I'd also say this is probably the job of early boot based on policy.

James

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

Re: [RFC PATCH 1/7] usb: typec: Generalize mux mode names

2018-05-08 Thread Heikki Krogerus 
Hi,

On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote:
> >> Even so, when the mux driver "set" function is called, it will just get the
> >> mode argument but since the mode (TYPEC_STATE_...) is overlapping for 
> >> different
> >> AMs if I understand your proposal correctly, the mux also needs to know 
> >> what AM
> >> is active.
> >> Does this imply that the mux set function signature need to change?
> > My plan was actually to propose we get rid of the current mux handling
> > (just leave the orientation switch) in favour of the notifications I'm
> > introducing with the type-c bus for the alternate modes. The current
> > mux handling is definitely not enough, and does not work in every
> > scenario, like also you pointed out.
> 
> So, the mux need to subscribe to each svid:mode pair it is interested in 
> using 
> typec_altmode_register_notifier() and then use those callbacks to switch the 
> correct
> signals to the connector. And a driver for an off-the-shelf mux device could 
> have
> the translation between svid:mode pairs and mux device specific control 
> specified by
> of/acpi properties. Right?

Yes. That is the plan. Would it work for you?


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 2/2] usb: misc: xapea00x: perform platform initialization of TPM

2018-05-08 Thread Jarkko Sakkinen 
On Fri, May 04, 2018 at 02:56:25PM -0500, David R. Bild wrote:
> On Fri, May 4, 2018 at 2:06 PM, Jason Gunthorpe  wrote:
> >
> > On Fri, May 04, 2018 at 08:00:22AM -0500, David R. Bild wrote:
> > > Normally the system platform (i.e., BIOS/UEFI for x86) is responsible
> > > for performing initialization of the TPM.  For these modules, the host
> > > kernel is the platform, so we perform the initialization in the driver
> > > before registering the TPM with the kernel TPM subsystem.
> >
> > The tpm driver already does most of this stuff automatically, why
> > duplicate it there and why is it coded in a way that doesn't use the
> > existing TPM services to do it?
> 
> I didn't want to have to duplicate all that functionality and was
> disappointed when that became the only option (due to the two reasons
> outlined below) for supporting existing kernels with an out-of-tree
> module.
> 
> Bringing the module in-tree opens the option of reworking some of the
> TPM subsystem to support this use case.  I'm open to concrete
> suggestions on how to do so.
> 
> 1) The first reason is that I don't think the necessary pieces are
> currently made available for reuse. I'd love to not repeat that code,
> but
> 
> - some required structs and functions are declared in private headers
> (drivers/char/tpm/*.h instead of include/linux/tpm.h).
> - many of the required functions are not exported.
> 
> If the TPM maintainers are open to more of the API being "public", I
> can look into preparing patches that export the necessary operations.
> 
> 2) The second reason is that the initialization done by the driver is
> work that should be done by platform, before the kernel ever sees the
> TPM.

This is too speculative to give any confirmitive promises. Do not fully
understand the reasoning. For example: why should I care about
out-of-tree modules? I can look code changes but the text above contains
too many words to nail anything down. I'm confused.

> In particular, it sets the credentials for the platform hierarchy.
> The platform hierarchy is essentially the "root" account of the TPM,
> so it's critical that those credentials be set before the TPM is
> exposed to user-space.  (The platform credentials aren't persisted in
> the TPM and must be set by the platform on every boot.)  If the driver
> registers the TPM before doing initialization, there's a chance that
> something else could access the TPM before the platform credentials
> get set.

Maybe. Not sure yet where to draw the line eg should TSS2 daemon to do
it for example.

James? Philip?

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 2/2] usb: misc: xapea00x: perform platform initialization of TPM

2018-05-08 Thread Jarkko Sakkinen 
On Fri, May 04, 2018 at 08:00:22AM -0500, David R. Bild wrote:
> Normally the system platform (i.e., BIOS/UEFI for x86) is responsible
> for performing initialization of the TPM.  For these modules, the host
> kernel is the platform, so we perform the initialization in the driver
> before registering the TPM with the kernel TPM subsystem.
> 
> The initialization consists of issuing the TPM startup command,
> running the TPM self-test, and setting the TPM platform hierarchy
> authorization to a random, unsaved value so that it can never be used
> after the driver has loaded.
> 
> Signed-off-by: David R. Bild 

Have you checked what the TPM driver already does?

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

Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

2018-05-08 Thread Oliver Neukum 
Am Freitag, den 04.05.2018, 08:59 + schrieb Jun Li:
> 
> > > > Can one implement a device that can operate as either DFP or UFP,
> > > > but not implements the dynamic role switch that a DRP must support?
> > > 
> > > You mean a port with DRD on data but not DRP on power?
> > > 
> > > The data-role is newly added as the data role is not coupled with
> > > power
> > 
> > No, I meant data role. As far as I can tell for a DRP you need to implement 
> > the
> > detection logic described in chapter 4 of the spec.
> 
> Could you please point me the "detection logic" of typec spec chapter 4
> you are referring to?

Chapter 4.5.2.2, especially state diagramms 4.15 and 4.16

It just seems to me that a DRP and a physical port that can be switched
between UFP and DFP are not the same thing, but can be implemented.

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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/2] usb: misc: xapea00x: add driver for Xaptum ENF Access Card

2018-05-08 Thread Oliver Neukum 
Am Montag, den 07.05.2018, 08:31 -0500 schrieb  David R. Bild :
> > > + spi_master->flags = 0;
> > > + spi_master->setup = xapea00x_spi_setup;
> > > + spi_master->transfer_one_message = 
> > > xapea00x_spi_transfer_one_message;
> > > +
> > > + retval = spi_register_master(spi_master);
> > > +
> > > + if (retval)
> > > + goto free_spi;
> > > +
> > > + dev->spi_master = spi_master;
> > 
> > Race condition.
> > 
> 
> What race condition do you see?  (I appreciate the review, but need
> some more specific help here.)

Hi,

you have registered the master. So it is functional, but if any
callback goes for dev->spi_master at that point, it will read an
incorrect value.

HTH
Oliver

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

Re: [usb-storage] [PATCH] usb: storage: Fix a possible data race in uas_queuecommand_lck

2018-05-08 Thread Jia-Ju Bai 



On 2018/5/8 16:27, Oliver Neukum wrote:

Am Dienstag, den 08.05.2018, 15:47 +0800 schrieb Jia-Ju Bai:

The write operations to "cmnd->result" and "cmnd->scsi_done"
are protected by the lock on line 642-643, but the write operations
to these data on line 634-635 are not protected by the lock.
Thus, there may exist a data race for "cmnd->result"
and "cmnd->scsi_done".

No,

the write operations need no lock. The low level driver at this point
owns the command. We cannot race with abort() for a command within
queuecommand(). We take the lock where we take it to protect
dev->resetting.

I don't see why the scope of the lock would need to be enlarged.


Okay, thanks for your reply and explanation.


Best wishes,
Jia-Ju Bai
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [usb-storage] [PATCH] usb: storage: Fix a possible data race in uas_queuecommand_lck

2018-05-08 Thread Oliver Neukum 
Am Dienstag, den 08.05.2018, 15:47 +0800 schrieb Jia-Ju Bai:
> The write operations to "cmnd->result" and "cmnd->scsi_done" 
> are protected by the lock on line 642-643, but the write operations 
> to these data on line 634-635 are not protected by the lock.
> Thus, there may exist a data race for "cmnd->result" 
> and "cmnd->scsi_done".

No,

the write operations need no lock. The low level driver at this point
owns the command. We cannot race with abort() for a command within
queuecommand(). We take the lock where we take it to protect
dev->resetting.

I don't see why the scope of the lock would need to be enlarged.

Regards
Oliver

> To fix this data race, the write operations on line 634-635 
> should be also protected by the lock.
> 
> Signed-off-by: Jia-Ju Bai 
Nacked-by: Oliver Neukum 

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

Re: [PATCH 4/5] arm64: allwinner: h6: add USB3 device nodes

2018-05-08 Thread Sergei Shtylyov 

Hello!

On 5/7/2018 6:18 PM, Icenowy Zheng wrote:


Allwinner H6 SoC features USB3 functionality, with a DWC3 controller and
a custom PHY.

Add device tree nodes for them.

Signed-off-by: Icenowy Zheng 
---
  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 38 
  1 file changed, 38 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi 
b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index c72da8cd9ef5..9564c938717c 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -174,6 +174,44 @@
status = "disabled";
};
  
+		usb3: usb@520 {


   I don't think  is allowed for a node having no "reg" prop...


+   compatible = "allwinner,sun50i-h6-dwc3";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges;
+   clocks = < CLK_BUS_XHCI>;
+   clock-names = "bus";
+   resets = < RST_BUS_XHCI>;
+   reset-names = "bus";
+   status = "disabled";
+
+   dwc3: dwc3 {


   Contrariwise, need  here...


+   compatible = "snps,dwc3";
+   reg = <0x520 0x1>;
+   interrupts = ;
+   /*
+* According to Wink from Allwinner, the
+* USB3 port on H6 is not capable of OTG;
+* the datasheet doesn't mention OTG at all
+* either, so the dr_mode is default to
+* "host" here.
+*/
+   dr_mode = "host";
+   phys = <>;
+   phy-names = "usb3-phy";
+   status = "disabled";
+   };
+   };

[...]

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

[PATCH] usb: storage: Fix a possible data race in uas_queuecommand_lck

2018-05-08 Thread Jia-Ju Bai 
The write operations to "cmnd->result" and "cmnd->scsi_done" 
are protected by the lock on line 642-643, but the write operations 
to these data on line 634-635 are not protected by the lock.
Thus, there may exist a data race for "cmnd->result" 
and "cmnd->scsi_done".

To fix this data race, the write operations on line 634-635 
should be also protected by the lock.

Signed-off-by: Jia-Ju Bai 
---
 drivers/usb/storage/uas.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6034c39b67d1..dde7a43ad491 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -627,17 +627,18 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
if (cmnd->device->host->host_self_blocked)
return SCSI_MLQUEUE_DEVICE_BUSY;
 
+   spin_lock_irqsave(>lock, flags);
+
if ((devinfo->flags & US_FL_NO_ATA_1X) &&
(cmnd->cmnd[0] == ATA_12 || cmnd->cmnd[0] == ATA_16)) {
memcpy(cmnd->sense_buffer, usb_stor_sense_invalidCDB,
   sizeof(usb_stor_sense_invalidCDB));
cmnd->result = SAM_STAT_CHECK_CONDITION;
cmnd->scsi_done(cmnd);
+   spin_unlock_irqrestore(>lock, flags);
return 0;
}
 
-   spin_lock_irqsave(>lock, flags);
-
if (devinfo->resetting) {
cmnd->result = DID_ERROR << 16;
cmnd->scsi_done(cmnd);
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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/2] usb: dwc2: alloc dma aligned buffer for isoc split in

2018-05-08 Thread wlf 

Dear Doug,

在 2018年05月08日 13:11, Doug Anderson 写道:

Hi,

On Mon, May 7, 2018 at 8:07 PM, William Wu  wrote:

+static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
+   struct dwc2_qh *qh,
+   struct dwc2_host_chan *chan)
+{
+   if (!hsotg->unaligned_cache)
+   return -ENOMEM;
+
+   if (!qh->dw_align_buf) {
+   qh->dw_align_buf = kmem_cache_alloc(hsotg->unaligned_cache,
+   GFP_ATOMIC | GFP_DMA);
+   if (!qh->dw_align_buf)
+   return -ENOMEM;
+
+   qh->dw_align_buf_size = min_t(u32, chan->max_packet,
+ DWC2_KMEM_UNALIGNED_BUF_SIZE);

Rather than using min_t, wouldn't it be better to return -ENOMEM if
"max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE?  As it is, you might
allocate less space than you need, right?  That seems like it would be
bad (even though this is probably impossible).

Yes, good idea! So is it good to fix it like this?

if (!qh->dw_align_buf || chan->max_packet >
DWC2_KMEM_UNALIGNED_BUF_SIZE)
return -ENOMEM; 

qh->dw_align_buf_size = chan->max_packet;




@@ -2797,6 +2837,32 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg 
*hsotg, struct dwc2_qh *qh)
 /* Set the transfer attributes */
 dwc2_hc_init_xfer(hsotg, chan, qtd);

+   /* For non-dword aligned buffers */
+   if (hsotg->params.host_dma && qh->do_split &&
+   chan->ep_is_in && (chan->xfer_dma & 0x3)) {
+   dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
+   if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
+   dev_err(hsotg->dev,
+   "Failed to allocate memory to handle non-aligned 
buffer\n");
+   /* Add channel back to free list */
+   chan->align_buf = 0;
+   chan->multi_count = 0;
+   list_add_tail(>hc_list_entry,
+ >free_hc_list);
+   qtd->in_process = 0;
+   qh->channel = NULL;
+   return -ENOMEM;
+   }
+   } else {
+   /*
+* We assume that DMA is always aligned in non-split
+* case or split out case. Warn if not.
+*/
+   WARN_ON_ONCE(hsotg->params.host_dma &&
+(chan->xfer_dma & 0x3));
+   chan->align_buf = 0;
+   }
+
 if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
 chan->ep_type == USB_ENDPOINT_XFER_ISOC)
 /*
@@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
 hsotg->params.dma_desc_enable = false;
 hsotg->params.dma_desc_fs_enable = false;
 }
+   } else if (hsotg->params.host_dma) {

Are you sure this is "else if"?  Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub?  Seems like this should be just "if".
Sorry, I don't understand the case "have descriptor DMA enabled in the 
controller and still need to do a normal DMA transfer". But maybe it 
still has another problem if just use "if" here, because it will create 
kmem caches for Slave mode which actually doesn't need aligned DMA buf.




+   /*
+* Create kmem caches to handle non-aligned buffer
+* in Buffer DMA mode.
+*/
+   hsotg->unaligned_cache = kmem_cache_create("dwc2-unaligned-dma",
+   DWC2_KMEM_UNALIGNED_BUF_SIZE, 4,

Worth using "DWC2_USB_DMA_ALIGN" rather than 4?



+   SLAB_CACHE_DMA, NULL);
+   if (!hsotg->unaligned_cache)
+   dev_err(hsotg->dev,
+   "unable to create dwc2 unaligned cache\n");
 }

 hsotg->otg_port = 1;
@@ -5279,6 +5356,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
  error4:
 kmem_cache_destroy(hsotg->desc_gen_cache);
 kmem_cache_destroy(hsotg->desc_hsisoc_cache);
+   kmem_cache_destroy(hsotg->unaligned_cache);

nitty nit: freeing order should be opposite of allocation, so the new
line should be above the other two.
Ah, I got it. But note that it's impossible to allocate the 
"unaligned_cache" and "desc *cache" at the same time. Should we still 
fix the free order? If yes, maybe the correct free order is:


kmem_cache_destroy(hsotg->unaligned_cache);
kmem_cache_destroy(hsotg->desc_hsisoc_cache);
kmem_cache_destroy(hsotg->desc_gen_cache);

Right?

And should we also need to fix the same free order in the "dwc2_hcd_remove"?
Best regards,
        wulf






--
To unsubscribe from this list: send the

Re: [PATCH v3 2/2] usb: dwc2: fix isoc split in transfer with no data

2018-05-08 Thread wlf 

Dear Doug,

在 2018年05月08日 13:13, Doug Anderson 写道:

Hi,

On Mon, May 7, 2018 at 8:07 PM, William Wu  wrote:

If isoc split in transfer with no data (the length of DATA0
packet is zero), we can't simply return immediately. Because
the DATA0 can be the first transaction or the second transaction
for the isoc split in transaction. If the DATA0 packet with no
data is in the first transaction, we can return immediately.
But if the DATA0 packet with no data is in the second transaction
of isoc split in transaction sequence, we need to increase the
qtd->isoc_frame_index and giveback urb to device driver if needed,
otherwise, the MDATA packet will be lost.

A typical test case is that connect the dwc2 controller with an
usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
headset) into the downstream port of Hub. Then use the usb mic
to record, we can find noise when playback.

In the case, the isoc split in transaction sequence like this:

- SSPLIT IN transaction
- CSPLIT IN transaction
   - MDATA packet (176 bytes)
- CSPLIT IN transaction
   - DATA0 packet (0 byte)

This patch use both the length of DATA0 and qtd->isoc_split_offset
to check if the DATA0 is in the second transaction.

Signed-off-by: William Wu 
---
Changes in v3:
- Remove "qtd->isoc_split_offset = 0" in the if test

Changes in v2:
- Modify the commit message

  drivers/usb/dwc2/hcd_intr.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index ba6fd852..3003594 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -930,9 +930,8 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg 
*hsotg,
 frame_desc = >urb->iso_descs[qtd->isoc_frame_index];
 len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
   DWC2_HC_XFER_COMPLETE, NULL);
-   if (!len) {
+   if (!len && !qtd->isoc_split_offset) {
 qtd->complete_split = 0;
-   qtd->isoc_split_offset = 0;
 return 0;
 }

This looks fine to me now, but as per my comments on the previous
version I don't think I've dug through this problem enough to add my
Reviewed-by tag.  I'll assume that John or someone with more knowledge
of the USB protocol than I have will Review / Ack.

Thanks very much for your review. Let's wait for other experts' suggestion.


-Doug






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