linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH] firewire: Add function to get speed from opaque struct fw_request
@ 2012-02-06 22:17 Chris Boot
  2012-02-07 18:15 ` Stefan Richter
  0 siblings, 1 reply; 3+ messages in thread
From: Chris Boot @ 2012-02-06 22:17 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel, Chris Boot, Clemens Ladisch, Stefan Richter

[ Would something like the following be acceptable as an addition to the
FireWire stack? This would be enough for me to get the speed of the
request for my work on the SBP-2 target code. ]

Sometimes it's useful to know the FireWire speed of the request that has
just come in to a fw_address_handler callback. As struct fw_request is
opaque we can't peek inside to get the speed out of the struct fw_packet
that's just inside. For example, the SBP-2 spec says:

"The speed at which the block write request to the MANAGEMENT_AGENT
register is received shall determine the speed used by the target for
all subsequent requests to read the initiator’s configuration ROM, fetch
ORB’s from initiator memory or store status at the initiator’s
status_FIFO. Command block ORB’s separately specify the speed for
requests addressed to the data buffer or page table."

[ ANSI T10/1155D Revision 4 page 53/54 ]

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: Clemens Ladisch <clemens@ladisch.de>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/core-transaction.c |    6 ++++++
 include/linux/firewire.h            |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 855ab3f..89c882d 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -820,6 +820,12 @@ void fw_send_response(struct fw_card *card,
 }
 EXPORT_SYMBOL(fw_send_response);
 
+int fw_request_speed(struct fw_request *request)
+{
+	return request->response.speed;
+}
+EXPORT_SYMBOL(fw_request_speed);
+
 static void handle_exclusive_region_request(struct fw_card *card,
 					    struct fw_packet *p,
 					    struct fw_request *request,
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 84ccf8e..eded4e4 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -340,6 +340,7 @@ int fw_core_add_address_handler(struct fw_address_handler *handler,
 void fw_core_remove_address_handler(struct fw_address_handler *handler);
 void fw_send_response(struct fw_card *card,
 		      struct fw_request *request, int rcode);
+int fw_request_speed(struct fw_request *request);
 void fw_send_request(struct fw_card *card, struct fw_transaction *t,
 		     int tcode, int destination_id, int generation, int speed,
 		     unsigned long long offset, void *payload, size_t length,
-- 
1.7.8.3


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [RFC][PATCH] firewire: Add function to get speed from opaque struct fw_request
  2012-02-06 22:17 [RFC][PATCH] firewire: Add function to get speed from opaque struct fw_request Chris Boot
@ 2012-02-07 18:15 ` Stefan Richter
  2012-02-07 19:41   ` Chris Boot
  0 siblings, 1 reply; 3+ messages in thread
From: Stefan Richter @ 2012-02-07 18:15 UTC (permalink / raw)
  To: Chris Boot; +Cc: linux1394-devel, linux-kernel, Clemens Ladisch

On Feb 06 Chris Boot wrote:
> [ Would something like the following be acceptable as an addition to the
> FireWire stack? This would be enough for me to get the speed of the
> request for my work on the SBP-2 target code. ]
[...]
> --- a/drivers/firewire/core-transaction.c
> +++ b/drivers/firewire/core-transaction.c
> @@ -820,6 +820,12 @@ void fw_send_response(struct fw_card *card,
>  }
>  EXPORT_SYMBOL(fw_send_response);
>  
> +int fw_request_speed(struct fw_request *request)
> +{
> +	return request->response.speed;
> +}
> +EXPORT_SYMBOL(fw_request_speed);
> +
>  static void handle_exclusive_region_request(struct fw_card *card,
>  					    struct fw_packet *p,
>  					    struct fw_request *request,
> diff --git a/include/linux/firewire.h b/include/linux/firewire.h
> index 84ccf8e..eded4e4 100644
> --- a/include/linux/firewire.h
> +++ b/include/linux/firewire.h
> @@ -340,6 +340,7 @@ int fw_core_add_address_handler(struct fw_address_handler *handler,
>  void fw_core_remove_address_handler(struct fw_address_handler *handler);
>  void fw_send_response(struct fw_card *card,
>  		      struct fw_request *request, int rcode);
> +int fw_request_speed(struct fw_request *request);
>  void fw_send_request(struct fw_card *card, struct fw_transaction *t,
>  		     int tcode, int destination_id, int generation, int speed,
>  		     unsigned long long offset, void *payload, size_t length,

Whenever you add a new exported function, please add a brief kerneldoc
comment right above its definition.  Can be a one-liner in this case.
Apart from this,

Acked-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

You can queue this via your SBP-2 target patch series (at which I haven't
looked yet), followed right away by a change to your driver which uses the
new symbol.  Or even better, fold these two changes into one.

Not sure if four more characters should be spent on the symbol's name
(fw_get_request_speed).  If you already thought about it and preferred
fw_request_speed, keep it that way.

There are at least two alternatives to your proposal:

  - We could move the definition of struct fw_request from
    drivers/firewire/core-transaction.c to include/linux/firewire.h.
    That would make for a rather bad driver API though.

  - We could expand the function type fw_address_callback_t() by a speed
    argument.  The other users of the API (firewire-core itself,
    firewire-sbp2, firewire-net, firedtv, snd-firewire-lib, and other
    as yet unmerged firewire sound drivers) and any other perceivable
    future user of the API would not care of that speed though.

I.e. the proposed "get speed of request" helper seems better.
-- 
Stefan Richter
-=====-===-- --=- --===
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RFC][PATCH] firewire: Add function to get speed from opaque struct fw_request
  2012-02-07 18:15 ` Stefan Richter
@ 2012-02-07 19:41   ` Chris Boot
  0 siblings, 0 replies; 3+ messages in thread
From: Chris Boot @ 2012-02-07 19:41 UTC (permalink / raw)
  To: Stefan Richter; +Cc: linux1394-devel, linux-kernel, Clemens Ladisch

On 07/02/2012 18:15, Stefan Richter wrote:
> On Feb 06 Chris Boot wrote:
>> [ Would something like the following be acceptable as an addition to the
>> FireWire stack? This would be enough for me to get the speed of the
>> request for my work on the SBP-2 target code. ]
> [...]
>> --- a/drivers/firewire/core-transaction.c
>> +++ b/drivers/firewire/core-transaction.c
>> @@ -820,6 +820,12 @@ void fw_send_response(struct fw_card *card,
>>   }
>>   EXPORT_SYMBOL(fw_send_response);
>>
>> +int fw_request_speed(struct fw_request *request)
>> +{
>> +	return request->response.speed;
>> +}
>> +EXPORT_SYMBOL(fw_request_speed);
>> +
>>   static void handle_exclusive_region_request(struct fw_card *card,
>>   					    struct fw_packet *p,
>>   					    struct fw_request *request,
>> diff --git a/include/linux/firewire.h b/include/linux/firewire.h
>> index 84ccf8e..eded4e4 100644
>> --- a/include/linux/firewire.h
>> +++ b/include/linux/firewire.h
>> @@ -340,6 +340,7 @@ int fw_core_add_address_handler(struct fw_address_handler *handler,
>>   void fw_core_remove_address_handler(struct fw_address_handler *handler);
>>   void fw_send_response(struct fw_card *card,
>>   		      struct fw_request *request, int rcode);
>> +int fw_request_speed(struct fw_request *request);
>>   void fw_send_request(struct fw_card *card, struct fw_transaction *t,
>>   		     int tcode, int destination_id, int generation, int speed,
>>   		     unsigned long long offset, void *payload, size_t length,
>
> Whenever you add a new exported function, please add a brief kerneldoc
> comment right above its definition.  Can be a one-liner in this case.
> Apart from this,
>
> Acked-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
>
> You can queue this via your SBP-2 target patch series (at which I haven't
> looked yet), followed right away by a change to your driver which uses the
> new symbol.  Or even better, fold these two changes into one.

I'll add some kerneldoc to it before I make it part of my series, it 
might as well be part of the same thing. Thanks for the ack!

> Not sure if four more characters should be spent on the symbol's name
> (fw_get_request_speed).  If you already thought about it and preferred
> fw_request_speed, keep it that way.

I did briefly consider it but I came up with 'fw_request_get_speed()' 
and didn't like it. fw_get_request_speed() makes sense to me though so 
I'll make the change before I send it out again.

> There are at least two alternatives to your proposal:
>
>    - We could move the definition of struct fw_request from
>      drivers/firewire/core-transaction.c to include/linux/firewire.h.
>      That would make for a rather bad driver API though.
>
>    - We could expand the function type fw_address_callback_t() by a speed
>      argument.  The other users of the API (firewire-core itself,
>      firewire-sbp2, firewire-net, firedtv, snd-firewire-lib, and other
>      as yet unmerged firewire sound drivers) and any other perceivable
>      future user of the API would not care of that speed though.
>
> I.e. the proposed "get speed of request" helper seems better.

Great!

Thanks,
Chris

-- 
Chris Boot
bootc@bootc.net

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2012-02-07 19:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-06 22:17 [RFC][PATCH] firewire: Add function to get speed from opaque struct fw_request Chris Boot
2012-02-07 18:15 ` Stefan Richter
2012-02-07 19:41   ` Chris Boot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).