Doug Maxey wrote:
> On Wed, 09 Aug 2006 13:52:07 EDT, Mike Christie wrote:
> ...
>>> @@ -836,9 +825,8 @@ static int qla4xxx_mem_alloc(struct scsi
>>> ha->srb_mempool = mempool_create(SRB_MIN_REQ, mempool_alloc_slab,
>>> mempool_free_slab, srb_cachep);
>>> if (ha->srb_mempool == NULL) {
>>> - ql4_printk(KERN_WARNING, ha,
>>> - "Memory Allocation failed - SRB Pool.\n");
>>> -
>>> + dev_warn(&ha->pdev->dev,
>>> + "Memory Allocation failed - SRB Pool.\n");
>>> goto mem_alloc_error_exit;
>> Sorry for the late response on this one. As you know I was out for a
>> while and I was waiting to get internet access yesterday.
>>
>> For these host messages, do we want something like the sdev_printk and
>> starget_printk or does it really make more sense to use the pci bus id
>> for the message prefix? What about other scsi host messages, will they
>> always go with the pci bus id or some scsi-ml id? And even if we want to
>> print out the pci bus id as the prefix instead of some scsi info, should
>> we still have some scsi wrapper?
>>
>
> I do agree that iscsi_transport sessions could use a new macro.
>
> My intention in this instance was to go with the widely used idiom, and
> to not have a driver specific one. Was trying to replace
> #define ql4_printk(level, ha, format, arg...) \
> dev_printk(level, &((ha)->pdev->dev), format, ## arg)
>
> But to follow on from irc, one more pass at this to help me clarify and
> understand what is need here.
>
> dev_xxx is a wrapper around dev_print(xxx ...).
>
> In the specific instance above, this should print
> scsiN arg...
I think the ones in qla4xxx_mem_alloc print
%pci_id: arg...
>
> Since qla4xxx_mem_alloc() is for the host, would an sdev_printk be
> the right thing? I don't believe we have any context of a target.
I do not think so. To answer the sdev_printk question for future
reference and maybe to clear things up, I think sdev_printk is for
struct scsi_devices so it should be used for messages about a scsi
device. I think where we may be missing each other on this is that sdev
is a common abbreviation for the scsi_device (as in scsi SPEC logic
unit). It is not a abbreviation for scsi device as in any old scsi
object that has a struct device in it or is a device that scsi messes
with. So maybe in functions like the device reset eh callback we could
use sdev_printk. For functions just interacting with the scsi host that
want to print something about the host you would not.
But to really answer what we should have been taking about, I think just
using dev_printk is probably ok for now. When you said in some other
mail, that you thought we needed to use dev_printk, I thought you meant
that we needed to covert all the messages to dev_printk or one of the
scsi wrappers around it. After reading the mail above that you are just
removing the needless ql4_printk wrapper, I think just replacing
ql4_printk usage with dev_printk is ok for now.
> Of course that may be a misunderstanding on my part.
>
> For this driver, I don't see any instances of a scsi_target.
>
The driver does not really deal with struct scsi_targets. The driver
interacts with structs like iscsi_sessions or iscsi_connections. The
iscsi_session hides most of the struct scsi_target details from the
driver. My mention of the scsi target was just because there is a
starget_printk and I was wondering if we need a shost_printk that the
LLD could use and that would complete the API.
For the iscsi conversion comment on irc, in scsi_transport_iscsi.c, we
do dev_printk(&session->dev and dev_printk(&connection->dev, and we
could maybe use some wrapper around them (like a iscsi_conn_printk() and
iscsi_sess_printk() that take a session and connection) like is done for
scsi_target and scsi_device structs and we could then use that in
libiscsi and iscsi_tcp and iscsi_iser. Right now in some of the iscsi
code we have some silly error messages that just spit out a error but do
not tell you what session, connection or host or device it is for.
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
