2022-03-04 07:54:25

by Jung Daehwan

[permalink] [raw]
Subject: [PATCH v1 1/4] usb: host: export symbols for xhci hooks usage

Export symbols for xhci hooks usage:
xhci_ring_free
- Allow xhci hook to free xhci_ring.

xhci_get_slot_ctx
- Allow xhci hook to get slot_ctx from the xhci_container_ctx
for getting the slot_ctx information to know which slot is
offloading and compare the context in remote subsystem memory
if needed.

xhci_get_ep_ctx
- Allow xhci hook to get ep_ctx from the xhci_container_ctx for
getting the ep_ctx information to know which ep is offloading and
comparing the context in remote subsystem memory if needed.

xhci_handle_event
- Allow xhci hook to handle the xhci events from the USB
controller.

xhci_update_erst_dequeue
- If xhci events was handle by xhci hook, it needs to update
the erst dequeue pointer to let the USB controller know the
events was handled.

xhci_ring_alloc
- Allocate a struct xhci_ring.

xhci_alloc_erst
xhci_free_erst
- Allocate and free event ring segment tables.

xhci_trb_virt_to_dma
- Used to retrieve the DMA address of a TRB

xhci_ring_cmd_db
- Notify the controller when a new command is issued

xhci_alloc_command
xhci_free_command
- Allocate and free a struct xhci_command

xhci_queue_stop_endpoint
- Issue a stop endpoint command to the controller

xhci_segment_free
- Free a segment struct.

xhci_link_segments
- Make the prev segment point to the next segment.

xhci_initialize_ring_info
- Initialize a ring struct.

xhci_check_trb_in_td_math
- Check TRB math for validation.

xhci_get_endpoint_address
- Get endpoint address from endpoint index.

xhci_address_device
- Issue an address device command

xhci_bus_suspend
xhci_bus_resume
- Suspend and resume for power scenario

xhci_remove_stream_mapping
- Remove stream mapping in stream endpoint

xhci_remove_segment_mapping
- Remove segment mapping

Signed-off-by: Daehwan Jung <[email protected]>
---
drivers/usb/host/xhci-hub.c | 2 ++
drivers/usb/host/xhci-mem.c | 29 +++++++++++++++++++++--------
drivers/usb/host/xhci-ring.c | 9 +++++++--
drivers/usb/host/xhci.c | 4 +++-
4 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index df3522dab31b..6c1b8d748d0f 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1809,6 +1809,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)

return 0;
}
+EXPORT_SYMBOL_GPL(xhci_bus_suspend);

/*
* Workaround for missing Cold Attach Status (CAS) if device re-plugged in S3.
@@ -1953,6 +1954,7 @@ int xhci_bus_resume(struct usb_hcd *hcd)
spin_unlock_irqrestore(&xhci->lock, flags);
return 0;
}
+EXPORT_SYMBOL_GPL(xhci_bus_resume);

unsigned long xhci_get_resuming_ports(struct usb_hcd *hcd)
{
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 0e312066c5c6..e6d56ef91ddb 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -65,7 +65,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
return seg;
}

-static void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
+void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
{
if (seg->trbs) {
dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
@@ -74,8 +74,9 @@ static void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
kfree(seg->bounce_buf);
kfree(seg);
}
+EXPORT_SYMBOL_GPL(xhci_segment_free);

-static void xhci_free_segments_for_ring(struct xhci_hcd *xhci,
+void xhci_free_segments_for_ring(struct xhci_hcd *xhci,
struct xhci_segment *first)
{
struct xhci_segment *seg;
@@ -96,9 +97,9 @@ static void xhci_free_segments_for_ring(struct xhci_hcd *xhci,
* DMA address of the next segment. The caller needs to set any Link TRB
* related flags, such as End TRB, Toggle Cycle, and no snoop.
*/
-static void xhci_link_segments(struct xhci_segment *prev,
- struct xhci_segment *next,
- enum xhci_ring_type type, bool chain_links)
+void xhci_link_segments(struct xhci_segment *prev,
+ struct xhci_segment *next,
+ enum xhci_ring_type type, bool chain_links)
{
u32 val;

@@ -118,6 +119,7 @@ static void xhci_link_segments(struct xhci_segment *prev,
prev->trbs[TRBS_PER_SEGMENT-1].link.control = cpu_to_le32(val);
}
}
+EXPORT_SYMBOL_GPL(xhci_link_segments);

/*
* Link the ring to the new segments.
@@ -206,7 +208,7 @@ static int xhci_insert_segment_mapping(struct radix_tree_root *trb_address_map,
return ret;
}

-static void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
+void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
struct xhci_segment *seg)
{
unsigned long key;
@@ -215,6 +217,7 @@ static void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
if (radix_tree_lookup(trb_address_map, key))
radix_tree_delete(trb_address_map, key);
}
+EXPORT_SYMBOL_GPL(xhci_remove_segment_mapping);

static int xhci_update_stream_segment_mapping(
struct radix_tree_root *trb_address_map,
@@ -256,7 +259,7 @@ static int xhci_update_stream_segment_mapping(
return ret;
}

-static void xhci_remove_stream_mapping(struct xhci_ring *ring)
+void xhci_remove_stream_mapping(struct xhci_ring *ring)
{
struct xhci_segment *seg;

@@ -269,6 +272,7 @@ static void xhci_remove_stream_mapping(struct xhci_ring *ring)
seg = seg->next;
} while (seg != ring->first_seg);
}
+EXPORT_SYMBOL_GPL(xhci_remove_stream_mapping);

static int xhci_update_stream_mapping(struct xhci_ring *ring, gfp_t mem_flags)
{
@@ -292,6 +296,7 @@ void xhci_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring)

kfree(ring);
}
+EXPORT_SYMBOL_GPL(xhci_ring_free);

void xhci_initialize_ring_info(struct xhci_ring *ring,
unsigned int cycle_state)
@@ -316,6 +321,7 @@ void xhci_initialize_ring_info(struct xhci_ring *ring,
*/
ring->num_trbs_free = ring->num_segs * (TRBS_PER_SEGMENT - 1) - 1;
}
+EXPORT_SYMBOL_GPL(xhci_initialize_ring_info);

/* Allocate segments and link them for a ring */
static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
@@ -407,6 +413,7 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
kfree(ring);
return NULL;
}
+EXPORT_SYMBOL_GPL(xhci_ring_alloc);

void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
struct xhci_virt_device *virt_dev,
@@ -519,6 +526,7 @@ struct xhci_slot_ctx *xhci_get_slot_ctx(struct xhci_hcd *xhci,
return (struct xhci_slot_ctx *)
(ctx->bytes + CTX_SIZE(xhci->hcc_params));
}
+EXPORT_SYMBOL_GPL(xhci_get_slot_ctx);

struct xhci_ep_ctx *xhci_get_ep_ctx(struct xhci_hcd *xhci,
struct xhci_container_ctx *ctx,
@@ -1755,6 +1763,7 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
INIT_LIST_HEAD(&command->cmd_list);
return command;
}
+EXPORT_SYMBOL_GPL(xhci_alloc_command);

struct xhci_command *xhci_alloc_command_with_ctx(struct xhci_hcd *xhci,
bool allocate_completion, gfp_t mem_flags)
@@ -1788,6 +1797,7 @@ void xhci_free_command(struct xhci_hcd *xhci,
kfree(command->completion);
kfree(command);
}
+EXPORT_SYMBOL_GPL(xhci_free_command);

int xhci_alloc_erst(struct xhci_hcd *xhci,
struct xhci_ring *evt_ring,
@@ -1818,6 +1828,7 @@ int xhci_alloc_erst(struct xhci_hcd *xhci,

return 0;
}
+EXPORT_SYMBOL_GPL(xhci_alloc_erst);

void xhci_free_erst(struct xhci_hcd *xhci, struct xhci_erst *erst)
{
@@ -1831,6 +1842,7 @@ void xhci_free_erst(struct xhci_hcd *xhci, struct xhci_erst *erst)
erst->erst_dma_addr);
erst->entries = NULL;
}
+EXPORT_SYMBOL_GPL(xhci_free_erst);

void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
@@ -1969,7 +1981,7 @@ static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
}

/* TRB math checks for xhci_trb_in_td(), using the command and event rings. */
-static int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
+int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
{
struct {
dma_addr_t input_dma;
@@ -2089,6 +2101,7 @@ static int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
xhci_dbg(xhci, "TRB math tests passed.\n");
return 0;
}
+EXPORT_SYMBOL_GPL(xhci_check_trb_in_td_math);

static void xhci_set_hc_event_deq(struct xhci_hcd *xhci)
{
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d0b6806275e0..2e99393560e5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -79,6 +79,7 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg,
return 0;
return seg->dma + (segment_offset * sizeof(*trb));
}
+EXPORT_SYMBOL_GPL(xhci_trb_virt_to_dma);

static bool trb_is_noop(union xhci_trb *trb)
{
@@ -311,6 +312,7 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
/* Flush PCI posted writes */
readl(&xhci->dba->doorbell[0]);
}
+EXPORT_SYMBOL_GPL(xhci_ring_cmd_db);

static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
{
@@ -2965,7 +2967,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
* Returns >0 for "possibly more events to process" (caller should call again),
* otherwise 0 if done. In future, <0 returns should indicate error code.
*/
-static int xhci_handle_event(struct xhci_hcd *xhci)
+int xhci_handle_event(struct xhci_hcd *xhci)
{
union xhci_trb *event;
int update_ptrs = 1;
@@ -3034,13 +3036,14 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
*/
return 1;
}
+EXPORT_SYMBOL_GPL(xhci_handle_event);

/*
* Update Event Ring Dequeue Pointer:
* - When all events have finished
* - To avoid "Event Ring Full Error" condition
*/
-static void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
+void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
union xhci_trb *event_ring_deq)
{
u64 temp_64;
@@ -3070,6 +3073,7 @@ static void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
temp_64 |= ERST_EHB;
xhci_write_64(xhci, temp_64, &xhci->ir_set->erst_dequeue);
}
+EXPORT_SYMBOL_GPL(xhci_update_erst_dequeue);

/*
* xHCI spec says we can get an interrupt, and if the HC has an error condition,
@@ -4420,6 +4424,7 @@ int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, struct xhci_command *cmd,
return queue_command(xhci, cmd, 0, 0, 0,
trb_slot_id | trb_ep_index | type | trb_suspend, false);
}
+EXPORT_SYMBOL_GPL(xhci_queue_stop_endpoint);

int xhci_queue_reset_ep(struct xhci_hcd *xhci, struct xhci_command *cmd,
int slot_id, unsigned int ep_index,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index dc357cabb265..041a65a6f175 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1449,6 +1449,7 @@ unsigned int xhci_get_endpoint_address(unsigned int ep_index)
unsigned int direction = ep_index % 2 ? USB_DIR_OUT : USB_DIR_IN;
return direction | number;
}
+EXPORT_SYMBOL_GPL(xhci_get_endpoint_address);

/* Find the flag for this endpoint (for use in the control context). Use the
* endpoint index to create a bitmask. The slot context is bit 0, endpoint 0 is
@@ -4306,10 +4307,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
return ret;
}

-static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
{
return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
}
+EXPORT_SYMBOL_GPL(xhci_address_device);

static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
{
--
2.31.1



2022-03-08 07:20:00

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v1 1/4] usb: host: export symbols for xhci hooks usage

On 04/03/2022 07:23, Daehwan Jung wrote:
> Export symbols for xhci hooks usage:
> xhci_ring_free
> - Allow xhci hook to free xhci_ring.

Instead of copying-pasting the name of function, please explain why do
you need these symbols exported.

The "Why" is actually one of most important questions, because "what is
this patch doing" we can easily see...

>
> xhci_get_slot_ctx
> - Allow xhci hook to get slot_ctx from the xhci_container_ctx
> for getting the slot_ctx information to know which slot is
> offloading and compare the context in remote subsystem memory
> if needed.
>


Best regards,
Krzysztof

2022-03-08 10:53:04

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v1 1/4] usb: host: export symbols for xhci hooks usage

Hi Daehwan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on krzk/for-next]
[also build test WARNING on char-misc/char-misc-testing v5.17-rc7 next-20220307]
[cannot apply to usb/usb-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Daehwan-Jung/usb-host-export-symbols-for-xhci-hooks-usage/20220304-143406
base: https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git for-next
config: hexagon-randconfig-r045-20220304 (https://download.01.org/0day-ci/archive/20220308/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/66bd02564cf7e666cf4c47f83965148fa5b90829
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daehwan-Jung/usb-host-export-symbols-for-xhci-hooks-usage/20220304-143406
git checkout 66bd02564cf7e666cf4c47f83965148fa5b90829
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/host/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/usb/host/xhci-mem.c:68:6: warning: no previous prototype for function 'xhci_segment_free' [-Wmissing-prototypes]
void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
^
drivers/usb/host/xhci-mem.c:68:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
^
static
drivers/usb/host/xhci-mem.c:79:6: warning: no previous prototype for function 'xhci_free_segments_for_ring' [-Wmissing-prototypes]
void xhci_free_segments_for_ring(struct xhci_hcd *xhci,
^
drivers/usb/host/xhci-mem.c:79:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void xhci_free_segments_for_ring(struct xhci_hcd *xhci,
^
static
drivers/usb/host/xhci-mem.c:100:6: warning: no previous prototype for function 'xhci_link_segments' [-Wmissing-prototypes]
void xhci_link_segments(struct xhci_segment *prev,
^
drivers/usb/host/xhci-mem.c:100:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void xhci_link_segments(struct xhci_segment *prev,
^
static
>> drivers/usb/host/xhci-mem.c:211:6: warning: no previous prototype for function 'xhci_remove_segment_mapping' [-Wmissing-prototypes]
void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
^
drivers/usb/host/xhci-mem.c:211:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
^
static
>> drivers/usb/host/xhci-mem.c:262:6: warning: no previous prototype for function 'xhci_remove_stream_mapping' [-Wmissing-prototypes]
void xhci_remove_stream_mapping(struct xhci_ring *ring)
^
drivers/usb/host/xhci-mem.c:262:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void xhci_remove_stream_mapping(struct xhci_ring *ring)
^
static
drivers/usb/host/xhci-mem.c:1984:5: warning: no previous prototype for function 'xhci_check_trb_in_td_math' [-Wmissing-prototypes]
int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
^
drivers/usb/host/xhci-mem.c:1984:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
^
static
6 warnings generated.


vim +/xhci_remove_segment_mapping +211 drivers/usb/host/xhci-mem.c

210
> 211 void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
212 struct xhci_segment *seg)
213 {
214 unsigned long key;
215
216 key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
217 if (radix_tree_lookup(trb_address_map, key))
218 radix_tree_delete(trb_address_map, key);
219 }
220 EXPORT_SYMBOL_GPL(xhci_remove_segment_mapping);
221
222 static int xhci_update_stream_segment_mapping(
223 struct radix_tree_root *trb_address_map,
224 struct xhci_ring *ring,
225 struct xhci_segment *first_seg,
226 struct xhci_segment *last_seg,
227 gfp_t mem_flags)
228 {
229 struct xhci_segment *seg;
230 struct xhci_segment *failed_seg;
231 int ret;
232
233 if (WARN_ON_ONCE(trb_address_map == NULL))
234 return 0;
235
236 seg = first_seg;
237 do {
238 ret = xhci_insert_segment_mapping(trb_address_map,
239 ring, seg, mem_flags);
240 if (ret)
241 goto remove_streams;
242 if (seg == last_seg)
243 return 0;
244 seg = seg->next;
245 } while (seg != first_seg);
246
247 return 0;
248
249 remove_streams:
250 failed_seg = seg;
251 seg = first_seg;
252 do {
253 xhci_remove_segment_mapping(trb_address_map, seg);
254 if (seg == failed_seg)
255 return ret;
256 seg = seg->next;
257 } while (seg != first_seg);
258
259 return ret;
260 }
261
> 262 void xhci_remove_stream_mapping(struct xhci_ring *ring)
263 {
264 struct xhci_segment *seg;
265
266 if (WARN_ON_ONCE(ring->trb_address_map == NULL))
267 return;
268
269 seg = ring->first_seg;
270 do {
271 xhci_remove_segment_mapping(ring->trb_address_map, seg);
272 seg = seg->next;
273 } while (seg != ring->first_seg);
274 }
275 EXPORT_SYMBOL_GPL(xhci_remove_stream_mapping);
276

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

2022-03-24 14:33:20

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v1 1/4] usb: host: export symbols for xhci hooks usage

On 23/03/2022 03:58, Jung Daehwan wrote:
> On Mon, Mar 07, 2022 at 10:59:06AM +0100, Krzysztof Kozlowski wrote:
>> On 04/03/2022 07:23, Daehwan Jung wrote:
>>> Export symbols for xhci hooks usage:
>>> xhci_ring_free
>>> - Allow xhci hook to free xhci_ring.
>>
>> Instead of copying-pasting the name of function, please explain why do
>> you need these symbols exported.
>>
>> The "Why" is actually one of most important questions, because "what is
>> this patch doing" we can easily see...
>>
>>>
>>> xhci_get_slot_ctx
>>> - Allow xhci hook to get slot_ctx from the xhci_container_ctx
>>> for getting the slot_ctx information to know which slot is
>>> offloading and compare the context in remote subsystem memory
>>> if needed.
>>>
>>
>>
>> Best regards,
>> Krzysztof
>>
>
> Hi Krzysztof
>
> xhci_ring_free has been removed from v3..
> The reason why I want to export is for managing vendor specific ring.
> I want to alloc and free vendor specific ring on specific address.
> It's done with xhci hooks.

It's better, but still does not explain why these have to be exported.
Please mention where are these hooks going to be. Where are they
implemented. I actually expect all of these exports to be used in your
patchset.

Best regards,
Krzysztof

2022-03-24 20:53:08

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v1 1/4] usb: host: export symbols for xhci hooks usage

On Wed, Mar 23, 2022 at 10:41:23AM +0100, Krzysztof Kozlowski wrote:
> On 23/03/2022 03:58, Jung Daehwan wrote:
> > On Mon, Mar 07, 2022 at 10:59:06AM +0100, Krzysztof Kozlowski wrote:
> >> On 04/03/2022 07:23, Daehwan Jung wrote:
> >>> Export symbols for xhci hooks usage:
> >>> xhci_ring_free
> >>> - Allow xhci hook to free xhci_ring.
> >>
> >> Instead of copying-pasting the name of function, please explain why do
> >> you need these symbols exported.
> >>
> >> The "Why" is actually one of most important questions, because "what is
> >> this patch doing" we can easily see...
> >>
> >>>
> >>> xhci_get_slot_ctx
> >>> - Allow xhci hook to get slot_ctx from the xhci_container_ctx
> >>> for getting the slot_ctx information to know which slot is
> >>> offloading and compare the context in remote subsystem memory
> >>> if needed.
> >>>
> >>
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >
> > Hi Krzysztof
> >
> > xhci_ring_free has been removed from v3..
> > The reason why I want to export is for managing vendor specific ring.
> > I want to alloc and free vendor specific ring on specific address.
> > It's done with xhci hooks.
>
> It's better, but still does not explain why these have to be exported.
> Please mention where are these hooks going to be. Where are they
> implemented. I actually expect all of these exports to be used in your
> patchset.

All exports _HAVE_ to be used by the patchset. Otherwise it's an
invalid submission.

thanks,

greg k-h

2022-03-25 18:19:15

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v1 1/4] usb: host: export symbols for xhci hooks usage

On 25/03/2022 02:28, Jung Daehwan wrote:
> On Wed, Mar 23, 2022 at 10:41:23AM +0100, Krzysztof Kozlowski wrote:
>> On 23/03/2022 03:58, Jung Daehwan wrote:
>>> On Mon, Mar 07, 2022 at 10:59:06AM +0100, Krzysztof Kozlowski wrote:
>>>> On 04/03/2022 07:23, Daehwan Jung wrote:
>>>>> Export symbols for xhci hooks usage:
>>>>> xhci_ring_free
>>>>> - Allow xhci hook to free xhci_ring.
>>>>
>>>> Instead of copying-pasting the name of function, please explain why do
>>>> you need these symbols exported.
>>>>
>>>> The "Why" is actually one of most important questions, because "what is
>>>> this patch doing" we can easily see...
>>>>
>>>>>
>>>>> xhci_get_slot_ctx
>>>>> - Allow xhci hook to get slot_ctx from the xhci_container_ctx
>>>>> for getting the slot_ctx information to know which slot is
>>>>> offloading and compare the context in remote subsystem memory
>>>>> if needed.
>>>>>
>>>>
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Hi Krzysztof
>>>
>>> xhci_ring_free has been removed from v3..
>>> The reason why I want to export is for managing vendor specific ring.
>>> I want to alloc and free vendor specific ring on specific address.
>>> It's done with xhci hooks.
>>
>> It's better, but still does not explain why these have to be exported.
>> Please mention where are these hooks going to be. Where are they
>> implemented. I actually expect all of these exports to be used in your
>> patchset.
>>
>> Best regards,
>> Krzysztof
>>
>
> OK. How about adding call stack like below?
>
> xhci_free_endpoint_ring -> xhci_vendor_free_transfer_ring(xhck hooks
> ops) -> xhci_ring_free

What I would like to see is high level explanation, why do you need
these functions exported. Such call trace does not answer this, because
you do not need to export functions just to call the. You need to export
them for modules, so this means that some modules (which - need names)
will be using these functions. Three or four sentences are usually
enough to explain it.


Best regards,
Krzysztof